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

chore: install psc-package via a NPM script #3386

Merged

Conversation

PenghaiZhang
Copy link
Contributor

@PenghaiZhang PenghaiZhang commented Sep 12, 2021

Checklist
  • the [contributor license agreement][] is signed
  • commit message follows [commit guidelines][]
  • tests are included
  • screenshots are included showing significant UI changes
  • documentation is changed or added
Description of change

With NPM 7 psc-package is not installed properly. So we created a new NPM script which will downloaded the package and use it.

This issue was originally found in renovate PRs (e.g. #3380 ) which failed to update package-lock.json.

We have been failing to install `psc-package` since we upgraded NPM to
version 7. So we should install the package via a NPM script.
Copy link
Member

@edalex-ian edalex-ian left a comment

Choose a reason for hiding this comment

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

Looks pretty good, may just want to tweak description to also mention this was triggered due to issues with renovate.

autotest/IntegTester/ps/package.json Outdated Show resolved Hide resolved
autotest/IntegTester/ps/package.json Outdated Show resolved Hide resolved
Copy link
Member

@edalex-ian edalex-ian left a comment

Choose a reason for hiding this comment

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

Cool, so just the one comment around the left over GHA step, and still need to pin those deps.

.github/workflows/ci.yaml Show resolved Hide resolved
@PenghaiZhang
Copy link
Contributor Author

Looks pretty good, may just want to tweak description to also mention this was triggered due to issues with renovate.

Can I mention this was triggered due to issues with renovate in the merge commit ?

@edalex-ian
Copy link
Member

Good to have both places. At least if you just duck up and edit the merge request description it'll be there nice and loud for if we need to reference back to it. And then if you put it in the merge commit it will be nicely there for future prosperity on the quite.

Copy link
Member

@edalex-ian edalex-ian left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@PenghaiZhang
Copy link
Contributor Author

Good to have both places. At least if you just duck up and edit the merge request description it'll be there nice and loud for if we need to reference back to it. And then if you put it in the merge commit it will be nicely there for future prosperity on the quite.

Cool. I have updated the PR description.

@PenghaiZhang
Copy link
Contributor Author

Ah GHA is still using NPM 6...

@PenghaiZhang
Copy link
Contributor Author

Failed on pulp build...

> pulp build

* Building project in /home/penghai/Edalex/forks/openEQUELLA/autotest/IntegTester/ps
* ERROR: `psc-package` executable not found.


Copy link
Member

@edalex-ian edalex-ian left a comment

Choose a reason for hiding this comment

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

Good job @PenghaiZhang ! Looks like you've pretty well won. 🎉

Just need to remove that bit of redundant code in install.js and then you should be GTG. 👍

- name: Setup node
uses: actions/setup-node@v2
with:
node-version: "${{ steps.nvm.outputs.NVMRC }}"
Copy link

@HonkingGoose HonkingGoose Sep 13, 2021

Choose a reason for hiding this comment

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

Did you know that setup-node comes with caching now?

Suggested change
node-version: "${{ steps.nvm.outputs.NVMRC }}"
node-version: "${{ steps.nvm.outputs.NVMRC }}"
cache: npm

Read the readme and learn more here: https://github.com/actions/setup-node#caching-packages-dependencies.

By the way setup-java comes with caching as well, but I don't know much about Java, so won't apply any suggestions here.

https://github.com/actions/setup-java#caching-packages-dependencies

Also check if the version of setup-java you're using comes with the caching capability before you turn it on for Java.

Copy link
Member

Choose a reason for hiding this comment

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

That's a nice new feature. Thanks for letting us know. We'll have a look and see what we might be able to refactor, because the build definition sure is long.

Choose a reason for hiding this comment

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

@edalex-ian Do you want me to make an issue with the same text? That way I'm not blocking this PR, and you can track the issue that way, and follow up when you have time to work on it.

Copy link
Member

Choose a reason for hiding this comment

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

That'd be great thanks @HonkingGoose. 👍

I'll go and merge this in the meantime. 😉

Choose a reason for hiding this comment

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

See issue #3390 for the "feature request".

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.

3 participants