-
-
Notifications
You must be signed in to change notification settings - Fork 183
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 support for Cirun on self-hosted GHA runners #1703
Conversation
bf49fcc
to
7b0c3e3
Compare
It's a bug from #1752 |
I don't think I have expertise to review this PR. However, can we make the docs PR sooner rather than later? I think that will help others figure out what to do with all of these new features. |
@conda-forge/core, ready for a review |
Same here but we can take a pragmatic approach by answering the following questions:
If both my answers above make sense then we can ask: Does it work for those testing it? Or do you need this merge to put it in production? If so, let's merge.
+100 |
Basic docs have been merged. There's a chance we need to write more, but for now I think they are sufficient to get us unblocked here and merge to try things. |
conda_smithy/configure_feedstock.py
Outdated
@@ -2008,6 +2091,17 @@ def _load_forge_config(forge_dir, exclusive_config_file, forge_yml=None): | |||
if config["test"] is None: | |||
config["test"] = "all" | |||
|
|||
if config["github_actions"]["cancel_in_progress"] is None: |
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.
What is the reasoning of this? Why bind cancel_in_progress
to whether the runners are self-hosted?
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.
To save CI resources by default. Self-hosted runners are usually going to be scarce or costly. If a user wants to not cancel-in-progress, they can explicitly say so, but by default it will cancel previous jobs when self-hosted runners are in use.
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 would also cancel hosted ones, too. They are scarce (only a limited pool of them) and can also be costly if you are not using your free minutes (or a runner that falls into the free minutes).
I would use the rather simple option of
if config["github_actions"]["cancel_in_progress"] is None:
config["github_actions"]["cancel_in_progress"] = True
Probably you can just set the default to False
?
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.
So, the cancel in progress here is configured to cancel all jobs in the same build. For eg: Linux builds will get cancelled if windows builds fail.
@@ -695,7 +698,6 @@ def _render_ci_provider( | |||
channel_target.startswith("conda-forge ") | |||
and provider_name == "github_actions" | |||
and not forge_config["github_actions"]["self_hosted"] | |||
and os.path.basename(forge_dir) not in SERVICE_FEEDSTOCKS |
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.
@jaimergp why was this line removed?
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 think we want it to be self-hosted or in service
Checklist
news
entrycupy
(DNM) cf-autotick-bot-test-package-feedstock#466