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

feat(ci): add status check for new workflow #290

Merged
merged 2 commits into from
Nov 1, 2023

Conversation

aali309
Copy link
Contributor

@aali309 aali309 commented Oct 31, 2023

fixes: #283

@aali309 aali309 added ci feat New feature or request safe-to-test labels Oct 31, 2023
@aali309
Copy link
Contributor Author

aali309 commented Oct 31, 2023

Copy link

ARCH IMAGE
amd64 ghcr.io/cryostatio/cryostat-core:pr-290-12aecf6971c78d528b22ae11ecfdcbe837790e18-linux-amd64
arm64 ghcr.io/cryostatio/cryostat-core:pr-290-12aecf6971c78d528b22ae11ecfdcbe837790e18-linux-arm64

To run smoketest:

# amd64          
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-core:pr-290-12aecf6971c78d528b22ae11ecfdcbe837790e18-linux-amd64 sh smoketest.sh

# or arm64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-core:pr-290-12aecf6971c78d528b22ae11ecfdcbe837790e18-linux-arm64 sh smoketest.sh

@aali309
Copy link
Contributor Author

aali309 commented Oct 31, 2023

/build_test

@mwangggg
Copy link
Member

ah it failed because an image hasn't been built for this specific ref for your PR.. perhaps the CI build workflow needs to be triggered on more PR events- I'll take a look

@mwangggg
Copy link
Member

mwangggg commented Oct 31, 2023

Oh it's because you ran /build_test before the CI build workflow was completed- so those images hadn't been pushed to ghcr.io yet. In core, the /build_test command isn't to build and test the Cryostat image build with the new -core, it just runs the integration tests. The building of the image is automatically triggered by the pull_request_target events.

@andrewazores
Copy link
Member

I guess putting the workflows in the same concurrency group and using cancel-in-progress: false could be a useful queuing behaviour for that kind of case, but I don't know if there would be other downsides associated with that, and I haven't put any thought into what to use as the concurrency group ID to tie those things together - I guess it would just have to be the branch name/checkout ref?

@aali309
Copy link
Contributor Author

aali309 commented Oct 31, 2023

oh it's because you ran /build_test before the CI build workflow was completed- so those images hadn't been pushed to ghcr.io yet

yes, I figured after looking at the build again and I deleted the comment

@mwangggg
Copy link
Member

mwangggg commented Oct 31, 2023

@andrewazores Hmm if they're in the same concurrency group and the /build_test workflow is running, I guess whomever's PR it is will have to wait until it's done to push any changes - could cause an old image to be tested if they're not careful. But otherwise, I can't think of any other downsides?

@aali309
Copy link
Contributor Author

aali309 commented Oct 31, 2023

@andrewazores Hmm if they're in the same concurrency group and the /build_test workflow is running, I guess whomever's PR it is will have to wait until it's done to push any changes - could cause an old image to be tested if they're not careful. But otherwise, I can't think of any other downsides?

This is what I ran into at first.

@andrewazores
Copy link
Member

Maybe there would be a way to group those things into a concurrency group based on the latest commit hash, so that when a new push comes in that has a new hash and is therefore in a new concurrency group/separate queue? I'm not sure if this is really worth the time investment to get working, it's going to come up pretty rarely and the workaround is just to wait for the initial build completion before invoking the build_test workflow.

@aali309
Copy link
Contributor Author

aali309 commented Oct 31, 2023

Maybe there would be a way to group those things into a concurrency group based on the latest commit hash, so that when a new push comes in that has a new hash and is therefore in a new concurrency group/separate queue? I'm not sure if this is really worth the time investment to get working, it's going to come up pretty rarely and the workaround is just to wait for the initial build completion before invoking the build_test workflow.

I agree about the time it might take to get this to work. May be something we can look at in the future?

@mwangggg
Copy link
Member

we could add a step that checks if the image exists, and if not, exit with a message that informs the commenter to wait until the initial build and push is completed to try integration testing?

@aali309
Copy link
Contributor Author

aali309 commented Oct 31, 2023

we could add a step that checks if the image exists, and if not, exit with a message that informs the commenter to wait until the initial build and push is completed to try integration testing?

yes, this was what I was thinking as well.

@mwangggg
Copy link
Member

I guess putting the workflows in the same concurrency group and using cancel-in-progress: false could be a useful queuing behaviour for that kind of case, but I don't know if there would be other downsides associated with that, and I haven't put any thought into what to use as the concurrency group ID to tie those things together - I guess it would just have to be the branch name/checkout ref?

Also, I'm not sure if it's the best idea to have cancel-in-progress: false for these workflows. There could be lots of bot comments from relabelling etc. i.e.: #281

@aali309
Copy link
Contributor Author

aali309 commented Nov 1, 2023

Took a lot of time, to conclude with this one. Many other solutions available but they will require a lot of code change. May be later we can look to make it like cryostat main to adopt the retest option?
image

Copy link

github-actions bot commented Nov 1, 2023

ARCH IMAGE
amd64 ghcr.io/cryostatio/cryostat-core:pr-290-224612dd3cdfc59ec613da5504cb77c6f732e905-linux-amd64
arm64 ghcr.io/cryostatio/cryostat-core:pr-290-224612dd3cdfc59ec613da5504cb77c6f732e905-linux-arm64

To run smoketest:

# amd64          
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-core:pr-290-224612dd3cdfc59ec613da5504cb77c6f732e905-linux-amd64 sh smoketest.sh

# or arm64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-core:pr-290-224612dd3cdfc59ec613da5504cb77c6f732e905-linux-arm64 sh smoketest.sh

@andrewazores
Copy link
Member

Looks pretty good to me. Rebase please

Copy link

github-actions bot commented Nov 1, 2023

ARCH IMAGE
amd64 ghcr.io/cryostatio/cryostat-core:pr-290-edc1af6c67152540f65b02cbcf265f6f0611b1c4-linux-amd64
arm64 ghcr.io/cryostatio/cryostat-core:pr-290-edc1af6c67152540f65b02cbcf265f6f0611b1c4-linux-arm64

To run smoketest:

# amd64          
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-core:pr-290-edc1af6c67152540f65b02cbcf265f6f0611b1c4-linux-amd64 sh smoketest.sh

# or arm64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-core:pr-290-edc1af6c67152540f65b02cbcf265f6f0611b1c4-linux-arm64 sh smoketest.sh

@andrewazores andrewazores merged commit a49647d into cryostatio:main Nov 1, 2023
13 checks passed
@aali309 aali309 deleted the addStatusCheck branch January 17, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci feat New feature or request safe-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task] Add status checks for new workflows
3 participants