Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve cicd #1424

Merged
merged 9 commits into from
Aug 2, 2022
Merged

Improve cicd #1424

merged 9 commits into from
Aug 2, 2022

Conversation

ildyria
Copy link
Member

@ildyria ildyria commented Jul 27, 2022

  • add a linter step to catch early php syntax error.
  • Add more structure and unify the CI.
  • Still run test even if code style fails.
  • Add support for artifact upload on master branch (which aims to simulate a master release build).

@codecov
Copy link

codecov bot commented Jul 29, 2022

Codecov Report

Merging #1424 (5bb6949) into master (1064f04) will decrease coverage by 0.81%.
The diff coverage is n/a.

Copy link
Collaborator

@nagmat84 nagmat84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only have limited understanding of Github workflow files. I checked out the old Build.yml and did a manual diff with the new CICD.yml on my local machine as Github does not recognize that CICD.yml is basically a merger of Build.yml and phpstan.yml plus some additions. From what I have seen it looks sane.

As I cannot say much about the workflow on the semantic level, I only have some less important comments about the syntax. I simply noticed some inconsistencies.

.github/workflows/CICD.yml Outdated Show resolved Hide resolved
.github/workflows/CICD.yml Outdated Show resolved Hide resolved
.github/workflows/CICD.yml Outdated Show resolved Hide resolved
.github/workflows/CICD.yml Outdated Show resolved Hide resolved
.github/workflows/CICD.yml Show resolved Hide resolved
@ildyria
Copy link
Member Author

ildyria commented Jul 30, 2022

I will standardize the strings with double quotes (even though they are not necessary per say).

@ildyria ildyria requested a review from nagmat84 July 31, 2022 07:59
@ildyria
Copy link
Member Author

ildyria commented Jul 31, 2022

The big difference is that there are dependencies between the different parts of the runs to potentially fail early.
https://github.com/LycheeOrg/Lychee/actions/runs/2768728603
image

I just hope that the build artifact step will work on master. This should allow us to have a "release-like" zip file built on each commit on master, potentially allowing us to give the "lastest" build from master without having to build a formal release. :)

Copy link
Collaborator

@nagmat84 nagmat84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With my little understand it looks good. Maybe we should just give it a try. 🙈

@nagmat84
Copy link
Collaborator

I will standardize the strings with double quotes (even though they are not necessary per say).

I believe you decided for without double quotes.

Copy link
Collaborator

@nagmat84 nagmat84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I just spotted that after I had a look on the dependency graph. I believe we want to check for syntax errors here.

.github/workflows/CICD.yml Show resolved Hide resolved
@ildyria
Copy link
Member Author

ildyria commented Jul 31, 2022

Addressed:

  • reordering of the task and dependency
  • removing superfluous apt
  • zip no longer only on master
  • zip the zip.

@ildyria ildyria requested a review from nagmat84 July 31, 2022 17:25
@d7415
Copy link
Contributor

d7415 commented Jul 31, 2022

Apparently GitHub Actions diagrams are terrible at showing dependencies clearly

(I accept that's a non-trivial task, and the interactive nature helps a lot, but it's still annoying)

@ildyria ildyria merged commit f39be7a into master Aug 2, 2022
@ildyria ildyria deleted the improve-CICD branch August 2, 2022 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants