Skip to content

Conversation

markpash
Copy link
Contributor

Duplicate the ginkgo workflow but with the race detection ci images.

Fixes: #26170

Duplicate the ginkgo workflow but with the race detection ci images.

Fixes: cilium#26170

Signed-off-by: Mark Pashmfouroush <mark@isovalent.com>
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 29, 2023
@markpash markpash marked this pull request as ready for review August 31, 2023 05:57
@markpash markpash requested review from a team as code owners August 31, 2023 05:57
@markpash markpash requested review from brlbil and qmonnet August 31, 2023 05:57
@markpash
Copy link
Contributor Author

I've duplicated the workflow, but not sure exactly how to test that this works as it should. I'd love some advice on how I can test this.

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

How will people remember to update the two workflows at the same time?

Given that the triggers are the same as the Ginkgo workflow, would it make sense instead to extend the Matrix to run with the race images as well, rather than duplicating the workflow entirely?

I'd love some advice on how I can test this.

Not sure if it will work, but from your changes in the ariane-config.yaml, let's try /ci-gingko?

# Warning: since this is a privileged workflow, subsequent workflow job
# steps must take care not to execute untrusted code.
- name: Checkout pull request branch (NOT TRUSTED)
uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
Copy link
Member

Choose a reason for hiding this comment

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

This version is outdated w.r.t. the current ginkgo workflow in the main branch.

Same for all other uses of actions/checkout, and for setup-go.

@qmonnet
Copy link
Member

qmonnet commented Aug 31, 2023

/ci-ginkgo

@markpash
Copy link
Contributor Author

How will people remember to update the two workflows at the same time?

Well the idea was to initially get this workflow in and running, and then once we know it works, figure out how to dedup it.

Given that the triggers are the same as the Ginkgo workflow, would it make sense instead to extend the Matrix to run with the race images as well, rather than duplicating the workflow entirely?

I'm not very opinionated on this, perhaps @nbusseneau has some ideas?

I'd love some advice on how I can test this.

Not sure if it will work, but from your changes in the ariane-config.yaml, let's try /ci-gingko?

Oh, I wasn't sure that I could run workflows from a fork.

@nbusseneau nbusseneau marked this pull request as draft August 31, 2023 15:23
@nbusseneau
Copy link
Member

nbusseneau commented Aug 31, 2023

It will indeed not run from a fork, you will need to re-open your PR with a branch located on upstream :) I would suggest using a separate trigger in Ariane config to be able to trigger only that workflow, and there is a special trick you need to do to get your new workflow to register when you open the PR: add a temporary pull_request: {} trigger to the workflow, just to register the workflow on the repository when the PR opens. After it has been registered, you will be able to run the workflow with the Ariane trigger no problem, and can remove the pull_request: {} trigger.

As for structure: for testing let's just do it copy/pasted for now, when we are happy with how it works I would suggest we look into merging the changes in the main Ginkgo workflow file, so that only one file needs to be maintained. One idea I have is to leverage the extra-args parameters that Ariane allows, so we could trigger race pipelines with a trigger like this: /ci-ginkgo race.

Let me know if you need help with anything :)

@github-actions
Copy link

github-actions bot commented Oct 1, 2023

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Oct 1, 2023
@nbusseneau nbusseneau closed this Oct 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add race-detector tests back to the CI pipeline
3 participants