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

Refresh workflows #216

Merged
merged 4 commits into from
Oct 20, 2022
Merged

Conversation

galleon
Copy link
Contributor

@galleon galleon commented Oct 12, 2022

Refreshing all actions used in both build and release workflows in prevision of node12 deprecation

Copy link
Contributor

@nhuet nhuet left a comment

Choose a reason for hiding this comment

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

You forgot to bump download-artifact in build.yml version at lines 424, 504, 584, 684, 717 and in release.yml (other lines:)
Be carefule that when using v3, you have also to add path argument when not yet present to make the next steps work:

      - name: Download artifacts
        uses: actions/download-artifact@v3
        with:
          name: wheels
          path: wheels

You could also change JamesIves/github-pages-deploy-action version in build.yml (for v4 as in release.yml) so that:

  • it is consistent with release.yml
  • more importantly it use the last v4.x.y version, which is now using node 16

Personnally i would rather squash all commit "Advancing..." and add explanation in commit message that it is to get node 16 using actions because of node 12 end of support.

I would like also having a small explanation about why you drop pre-commit action and change the action creating the release. (I know why because I did the same for discrete-optimization but someone else might want to know the reason requiring these changes).

@galleon galleon force-pushed the galleon/refresh_workflows branch from 6fbe65b to 303a786 Compare October 13, 2022 20:30
@galleon galleon force-pushed the galleon/refresh_workflows branch from 303a786 to ae7cfa7 Compare October 13, 2022 20:43
@galleon galleon marked this pull request as ready for review October 13, 2022 20:43
Copy link
Contributor

@nhuet nhuet left a comment

Choose a reason for hiding this comment

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

LGTM

@dbarbier
Copy link
Contributor

There are still many warnings

@nhuet
Copy link
Contributor

nhuet commented Oct 14, 2022

There are still many warnings

This is another matter: these warnings (at least those I have seem) are not about node 12, but rather about "set-output command is deprecated". And are actually coming from github actions (maintained by github) for which the version used is already the more recent. For instance, download-artifact@v3 leads to these warnings.

My guess is that github will soon update these actions and that we will have to bump them again at that time.

@dbarbier
Copy link
Contributor

There are still many warnings

This is another matter: these warnings (at least those I have seem) are not about node 12, but rather about "set-output command is deprecated". And are actually coming from github actions (maintained by github) for which the version used is already the more recent. For instance, download-artifact@v3 leads to these warnings.

My guess is that github will soon update these actions and that we will have to bump them again at that time.

No, these warnings come from our workflows, grep "set-output".

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
@galleon galleon force-pushed the galleon/refresh_workflows branch 2 times, most recently from 1086824 to 1a2ef74 Compare October 16, 2022 09:43
@galleon
Copy link
Contributor Author

galleon commented Oct 16, 2022

The remaining warnings come from actions/download-artifact@v3. There is a PR to update to the latest actions/core that will solve this - not is our hands.

@galleon galleon requested review from nhuet and dbarbier and removed request for nhuet and dbarbier October 16, 2022 09:48
Copy link
Contributor

@dbarbier dbarbier left a comment

Choose a reason for hiding this comment

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

IMHO you should check release.yml in your fork

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
with open(environ["GITHUB_OUTPUT"], "a") as f:
f.write(f"build={dumps(build_dict)}\n")
f.write(f"test={dumps(dict({os : [k for k in test if k.startswith(os)] for os in oses}))}\n")
f.write(f"build_doc={dumps(build_doc)}\n")
Copy link
Contributor

@dbarbier dbarbier Oct 16, 2022

Choose a reason for hiding this comment

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

You are modifying the contents of these variables without giving any reason; why did you add calls to dumps()?

@galleon galleon force-pushed the galleon/refresh_workflows branch 2 times, most recently from 4fcfc2a to 4c5a4df Compare October 16, 2022 20:03
Copy link
Contributor

@dbarbier dbarbier left a comment

Choose a reason for hiding this comment

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

Looks good, but you must explain in the last commit message why you want to merge these changes. You claim that set-output is unsafe, this is not clear to me whether this is true.

@galleon galleon force-pushed the galleon/refresh_workflows branch from 4c5a4df to 2c00ed2 Compare October 17, 2022 08:04
@galleon
Copy link
Contributor Author

galleon commented Oct 17, 2022

Looks good, but you must explain in the last commit message why you want to merge these changes. You claim that set output is unsafe, this is not clear to me whether this is true.
I have used GH comment

@galleon galleon requested review from dbarbier and removed request for nhuet October 17, 2022 08:08
@galleon galleon requested review from nhuet and removed request for dbarbier October 17, 2022 08:08
Copy link
Collaborator

@g-poveda g-poveda left a comment

Choose a reason for hiding this comment

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

trust in previous reviews.

@g-poveda g-poveda merged commit 3ae94b8 into airbus:master Oct 20, 2022
@galleon galleon deleted the galleon/refresh_workflows branch October 21, 2022 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants