-
Notifications
You must be signed in to change notification settings - Fork 95
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
Simplify CI #1819
Simplify CI #1819
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.
In general, I'm ok with that. However, that means everyone that needs to do more than basic stuff, needs to have push access to the repo in order to send PRs that aren't coming from a fork. If we want that is open for discussion, but this at least needs to be documented.
Last time we spoke, I mentioned that PyTorch has a way to "spawn" CI jobs by tagging a PR. Assuming I dig up how they do it, is this something we want? Because this PR seems to make it impossible.
@pmeier thanks for the feedback! The main issue with the In my personal opinion, at this point, I don't think the maintenance burden of this additional workflow outweighs the potential upside of catching an error (which is now more easily visible from the repo README). This is a tradeoff I am willing to make but I'm eager to hear what other people have to say. |
After some offline chat with @iameskild, I've implemented my suggestion from #1819 (comment) in 9fff954. However, since this PR is coming from a fork, it is impossible to test here 😄 So we either merge as is (after review of course, but untested by CI) and fix later if something comes up, or we revert the patch again and merge it after the main PR has been merged. |
This looks great, and after pondering around it for some time, it does make sense that the cloud provider tests run daily instead of per contributor PR. The reason for it is that pytest (specifically the
This should not block a contribution for both scenarios as it's not part of the contributor's responsibility. So, with that in mind, my only suggestion to change is to update the trigger of the tests workflow to only run for a contributor in case there were changes to the tests themselves. This is, these need to be scoped down a bit: - "tests/**"
- "tests_deployment/**"
- "tests_e2e/**"
- "scripts/**"
- "nebari/**"
- "pyproject.toml" For example, from those above, I only see the need to run in case |
Thanks for the feedback @viniciusdc! I paired down the trigger paths for |
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.
Need a decision on #1819 (comment), but otherwise good to go from my side. Thanks Eskild!
CI/CD LucidCharts have been updated. This is ready to merge. |
Reference Issues or PRs
We've spent a decent amount of time trying to get CI working for PRs that come from a forked repo as well as maintaining CI workflows (such as Infracost) that are essentially never used. In this PR, I propose we simplify our CI setup as much as possible to reduce the maintenance burden. This PR eliminates the need to maintain the
Contributor
and
Infracost
workflows.From the perspective of maintainers and developers, nothing should change. The only noticeable differences are that we now explicitly agree that we won't run the
Test Nebari Providers
on PRs coming from folks (this was never working before so this isn't really a change but more of an affirmation that this expected behavior) and instead run this test on a daily basis as a general precaution. This PR itself was opened from my fork of the repo so this is what we expect new contributors to see in terms of CI; first time contributors will still need a maintainer to approve and run CI.What does this implement/fix?
Put a
x
in the boxes that applyTesting
Any other comments?