-
Notifications
You must be signed in to change notification settings - Fork 498
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
github actions from v3 to v4 #10997
github actions from v3 to v4 #10997
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
conf/solr/update-fields.sh
Outdated
@@ -22,6 +22,7 @@ COPY_FIELDS="" | |||
TRIGGER_CHAIN=0 | |||
ED_DELETE_FIELDS="'a+,'b-d" | |||
ED_DELETE_COPYFIELDS="'a+,'b-d" | |||
FAKEDONOTMERGE=0 |
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
6 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@stevenwinship this pull request looks fine if we want to do the same thing as last time... merge it, see if anything breaks, and revert it if necessary. I see 15 commits. Did you do some testing? If so, can you please write a little about it? "Suggestions on how to test this" is empty. Is this because we plan to just merge it and see if anything breaks? |
The reason for so many commits was indeed testing. I wanted to test with updating all yml files to V4 but ran into some issues so I ended up limiting the changes to the 2 files that were failing. I also tested with java changes and doc changes to force all the tests/actions to 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.
@stevenwinship thanks for the explanation. Here's one more question before I approve this.
|
||
- name: Download war artifact | ||
uses: actions/download-artifact@v3 | ||
uses: actions/download-artifact@v4.1.7 |
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.
Is there a reason why "upload artifact" is set to "v4" but "download-artifact" is a specific version (v4.1.7)? Should we put a comment above the 4.1.7 lines to explain why it's pinned so specifically?
There's one case here and two more in the other file.
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 mentioned in an issue that there was a bug in download V4 that was fixed in 4.1.7 actions/upload-artifact#478
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.
added comment
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 also discussion at IQSS/dataverse-frontend#550 (comment)
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 before, we'll merge this and check if it breaks anything and revert if necessary.
Also, I assumed "v4" meant "the latest v4 but it doesn't. See https://github.com/orgs/community/discussions/25248 That is, v4 means https://github.com/actions/download-artifact/releases/tag/v4 v4 does not meant the latest, which as of this writing, is https://github.com/actions/download-artifact/releases/tag/v4.1.8 |
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.
Looks good. As discussed at IQSS/dataverse-frontend#550 (comment) we've learned that "v4" really does mean "the latest 4" as I initially assumed.
Approved.
Let's revert this if it breaks anything.
Merging PR - monitoring github actions |
It's passing now. Those failures were probably unrelated. I think it's safe to move this to "done". |
What this PR does / why we need it: Starting November 30, 2024, GitHub Actions customers will no longer be able to use v3 of actions/upload-artifact or actions/download-artifact.
Which issue(s) this PR closes:#10993
Special notes for your reviewer: I chose to update all the V3 to V4 actions in the yml files. Only the upload and download were required but I felt it's better to not mix/load multiple library versions if possible.
Due to a bug in download-artifact@V4 the version was bumped to 4.1.7 to include the fix
As mentioned in an issue that there was a bug in download V4 that was fixed in 4.1.7 actions/upload-artifact#478
Update: v4 now gets the latest so download-artifact can now use v4 to get the fix in 4.1.7 (latest is v4.1.8 as of this PR)
Suggestions on how to test this: I've made temporary changes to various files to force most of the actions to run. Obviously any release specific actions were not run. I think the best way to test it is to merge it and monitor the build system.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: