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

Add Github Actions #721

Merged
merged 1 commit into from
Jun 5, 2024
Merged

Add Github Actions #721

merged 1 commit into from
Jun 5, 2024

Conversation

cdesiniotis
Copy link
Contributor

No description provided.

Copy link
Member

@elezar elezar left a 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).

tests/holodeck.yaml Outdated Show resolved Hide resolved
- 0.0.0.0/0
image:
architecture: amd64
imageId: ami-0ce2cb35386fc22e9
Copy link
Collaborator

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 :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

.github/workflows/ci.yaml Show resolved Hide resolved
@ArangoGutierrez ArangoGutierrez added the testing issue/PR to fix/edit/create/enhance a project unit/e2e test label May 15, 2024
@cdesiniotis
Copy link
Contributor Author

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).

In general I agree we should split the actions and align common workflows, like golang.yaml and image.yaml, across all of our projects. And @ArangoGutierrez I know we discussed your plans for removing some of the current limitations, like not being able to run e2e tests on PRs, in the future. However, are you okay if we keep this PR as is for the time being so that we mimic the existing pipelines on Gitlab as much as possible? I am happy to re-factor this into multiple files and align with other projects once the limitations are addressed.

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:

if: github.event.pull_request.head.repo.full_name == "github.com/NVIDIA/gpu-operator"

@ArangoGutierrez ArangoGutierrez marked this pull request as ready for review May 22, 2024 18:31
Copy link
Collaborator

@ArangoGutierrez ArangoGutierrez left a 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

.github/workflows/ci.yaml Show resolved Hide resolved
.github/workflows/ci.yaml Show resolved Hide resolved
@ArangoGutierrez
Copy link
Collaborator

@cdesiniotis holodeck is fixed, now it fails for real reasons

Signed-off-by: Christopher Desiniotis <[email protected]>
@cdesiniotis
Copy link
Contributor Author

@cdesiniotis holodeck is fixed, now it fails for real reasons

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.

@tariq1890
Copy link
Contributor

tariq1890 commented May 28, 2024

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:
if: github.event.pull_request.head.repo.full_name == "github.com/NVIDIA/gpu-operator"

I think we can get around this by requiring manual approves by the code owners for e2e github pipelines in PRs at all times.

@cdesiniotis
Copy link
Contributor Author

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
Copy link
Contributor

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 ?

Copy link
Contributor Author

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?

Copy link
Member

@elezar elezar May 31, 2024

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

.github/workflows/ci.yaml Show resolved Hide resolved
.github/workflows/ci.yaml Show resolved Hide resolved
@elezar elezar dismissed ArangoGutierrez’s stale review May 31, 2024 12:29

Comments addressed.

Copy link
Member

@elezar elezar left a 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.

@cdesiniotis cdesiniotis merged commit e2de7e0 into master Jun 5, 2024
11 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing issue/PR to fix/edit/create/enhance a project unit/e2e test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants