Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MVP improvements to automated archive runs #357
MVP improvements to automated archive runs #357
Changes from 32 commits
7ca2750
13302d9
9425dd5
4cf7715
9610964
bf58f1c
68c6edf
83961e3
60a76c5
dd270b4
a2cb688
f304b4e
ce9158e
7cb879a
3d31147
53c12d4
6242c47
8bbdf9c
6094cb4
a7622b0
ec4eea9
ad7464c
5883670
71bd627
4bf7d5c
a1301a0
4829753
0d51e24
0e015c6
6fdd347
1fe8ba0
f179acc
02d0eb6
844ecc2
140473c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
non-blocking: we could maybe strip this whole section if there's that
summary of results
section above.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.
If scheduled, should default to the full list.
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.
blocking: What do you think of defining one list of "all the damn datasets" in
env
so we can access that everywhere we need to?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.
It's a bit tricky here because there should in fact be two variables - "small runner datasets" and "large runner datasets". The alternative is one variable with some kind of filtering, but I couldn't figure out how to do that neatly. But I can make a "small" and "large" dataset list.
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.
If set as true in workflow dispatch, or triggered by scheduled run this should run.
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.
Hm, if triggered by scheduled run, I would expect
inputs.large_runner
be empty and thusarchive-run-large
to get skipped - am I missing something here?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.
My assumption was that an empty string would actually get evaluated as true, but I could be totally off-base here. I think your suggestion re: incorporating the type of run below is great and I'll incorporate it here.
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 an unset variable here would get treated as a "falsey" value: https://docs.github.com/en/actions/learn-github-actions/expressions#literals
And actually I bet unset variable is actually
null
instead of''
, now that I look at those docs.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.
Either way, making this more explicit seems wise.
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.
See discussion here for use of
always()
actions/runner#491Completely unhinged GHA behavior, but what can we do.
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.
Whee! hey, if it works it works.
Is the idea behind
inputs.create_github_issue != false
that"" != false
and then we will get the github issue in scheduled runs as well as the specified manual runs?If so, what do you think of using
github.event_name
to differentiate betweenworkflow_dispatch
andscheduled
runs? That is more explicitly "do X step if it's scheduled or if there's some specific workflow_dispatch input."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 that's a great idea, can incorporate it here. But yes, that was my original idea.