-
Notifications
You must be signed in to change notification settings - Fork 311
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
Add Github Actions #721
Add Github Actions #721
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general comment, does it make sense to split the actions based on scope. For example in the device plugin we have:
.github/workflows
├── e2e.yaml
├── golang.yaml
├── image.yaml
└── stale.yaml
1 directory, 4 files
Here golang.yaml
and image.yaml
are generally generic enough to reuse across all our components. We can add a separate file for the other validation (linting etc).
- 0.0.0.0/0 | ||
image: | ||
architecture: amd64 | ||
imageId: ami-0ce2cb35386fc22e9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upcoming holodeck
release won't require this line :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I agree we should split the actions and align common workflows, like To prevent e2e tests from running on forks (which will likely fail anyways if AWS creds are not configured on the fork), we can consider adding the below conditional to the e2e jobs:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add this line to ensure we only run e2e on merge for now
@cdesiniotis |
Signed-off-by: Christopher Desiniotis <[email protected]>
Thanks. The e2e jobs are failing because the container images for this repo have not been made public yet. I have pinged George to make them public. |
I think we can get around this by requiring manual approves by the code owners for e2e github pipelines in PRs at all times. |
All checks have passed. @ArangoGutierrez @elezar could you take another look? For now, the Github Actions resemble our Gitlab pipelines. In a follow-up, we can look to restrict when the e2e test jobs run. |
apiVersion: holodeck.nvidia.com/v1alpha1 | ||
kind: Environment | ||
metadata: | ||
name: HOLODECK_NAME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we choose a different name here? Perhaps, something along the lines of gpu-operator-e2e-env ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the holodeck action automatically generates a name, but I could be wrong. @ArangoGutierrez what is the recommendation for naming this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is the same as for the device plugin, let's leave it as is for now -- pending guidance on what this should be.
To clarify, the name is set here for the github action: https://github.com/NVIDIA/holodeck/blob/58fe703797a66510af3598bfca139fbaa46f47fa/cmd/action/ci/ci.go#L58-L69
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cdesiniotis. I'm happy to start with this and iterate once we have the initial kinks ironed out.
No description provided.