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

Added version check to CI checks #545

Merged
merged 1 commit into from
Sep 21, 2023
Merged

Conversation

dheerajd5
Copy link
Contributor

This PR adds a version-check to CI checks. It takes the version fetched from git and from the file version.h and compares it.
#497

scripts/ci_checks/linux-checks/check-version.sh Outdated Show resolved Hide resolved
scripts/ci_checks/linux-checks/check-version.sh Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
scripts/ci_checks/linux-checks/check-version.sh Outdated Show resolved Hide resolved
@adrianopol adrianopol added the needs revision Pull request should be revised by its author label Sep 5, 2023
.github/workflows/build.yml Outdated Show resolved Hide resolved
@gavv
Copy link
Member

gavv commented Sep 6, 2023

Two small comments:

  • we shouldn't run this check unless we're building a tag naned as v* - because we usually update header before creating a tag

  • ideally we shouldn't checkout full history unless we're doing this check, to avoid slowing down regular builds

Copy link
Member

@adrianopol adrianopol left a comment

Choose a reason for hiding this comment

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

LGTM; also pay attention to gavv's comments please.
Thank you!

@gavv
Copy link
Member

gavv commented Sep 6, 2023

@dheerajd5 As you mentioned in the issue, android failure is not related to your pr. I restarted that job.

@dheerajd5
Copy link
Contributor Author

dheerajd5 commented Sep 7, 2023

Two small comments:

  • we shouldn't run this check unless we're building a tag naned as v* - because we usually update header before creating a tag
  • ideally we shouldn't checkout full history unless we're doing this check, to avoid slowing down regular builds

Hey, So I looked into conditional execution in Actions, to check if we're building a tag with v*, we can probably check the latest commit message, and see if the string matches a v* format.
And for the second point, we can pass an if condition to see what kind of check we're going to run and based of that if it is necessary to pull the full history. I will try to implement the solution to the second point.

Edit: Removed the note asking about to how rebase without coauthoring the commit.

@dheerajd5
Copy link
Contributor Author

dheerajd5 commented Sep 7, 2023

I was looking into the first point, I'm not really sure how to go about it do I check the latest commit and message and try to find a v* matching string?

@dheerajd5 dheerajd5 reopened this Sep 7, 2023
@dheerajd5 dheerajd5 force-pushed the develop branch 2 times, most recently from f0556b2 to c92258f Compare September 7, 2023 06:12
@gavv
Copy link
Member

gavv commented Sep 7, 2023

I think we should extract this from linux-checks into separate job. Only that job will do deep checkout and fetch tags, and other jobs won't be affected. E.g. we can call it release-checks.

The new job then can be enabled conditionally only for properly named tags. Here is how you can do it: https://github.com/roc-streaming/roc-go/blob/6212890ef56528624350571ce13f308e0a3ae11c/.github/workflows/build.yaml#L219C53-L219C53

@dheerajd5
Copy link
Contributor Author

There are also ways to conditionally execute steps in a job, that's what I've done. Although the solution is a bit hard coded, the workflow only executes the step, fetch tag, if the script that is going to be run is version-check.

But in this case, there was no way that I was aware of where I could conditionally run it based on the tag name.

I'll go ahead with the solution you suggested.

@dheerajd5
Copy link
Contributor Author

dheerajd5 commented Sep 12, 2023

Hey, so I implemented the changes, but it's not passing a few unrelated tests. I don't really have an idea why, Namely, the macos13 series, Here's the workflow run https://github.com/dheerajd5/roc-toolkit/actions/runs/6157211700 and here's the commit page, (dheerajd5@64f2fd6)

@gavv
Copy link
Member

gavv commented Sep 12, 2023

Hey, so I implemented the changes, but it's not passing a few unrelated tests. I don't really have an idea why, Namely, the macos13 series

Could you please fetch fresh develop and rebase on it? Should be fixed now.

Added conditional execution for check-version script

Extracting check-version into release checks job
@dheerajd5
Copy link
Contributor Author

Hey so I did what you told, it has passed all the checks now. It hasn't run the check I've written cause I didn't upload this with a version tag.

@gavv gavv merged commit 8280b62 into roc-streaming:develop Sep 21, 2023
32 checks passed
@gavv
Copy link
Member

gavv commented Sep 21, 2023

Thanks. Tested it in a fork, there were problems.

Follow-up commit with fixes: 0266785

Changes:

@gavv gavv removed the needs revision Pull request should be revised by its author label Sep 21, 2023
@dheerajd5
Copy link
Contributor Author

Ahh, sorry I couldn't submit a more complete code, but enjoyed working, thanks guys @gavv @adrianopol

@gavv
Copy link
Member

gavv commented Sep 21, 2023

You're welcome!

@gavv gavv added the contribution A pull-request by someone else except maintainers label Oct 2, 2023
@gavv gavv added this to the 0.3.0 milestone Nov 17, 2023
gavv pushed a commit to gavv/roc-toolkit that referenced this pull request Nov 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution A pull-request by someone else except maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants