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

Bump @actions/core from v1.9.1 to v1.10.0 #101

Merged
merged 3 commits into from
Nov 11, 2022
Merged

Bump @actions/core from v1.9.1 to v1.10.0 #101

merged 3 commits into from
Nov 11, 2022

Conversation

ericcornelissen
Copy link
Contributor

@ericcornelissen ericcornelissen commented Oct 16, 2022

Closes #100 by upgrading @actions/core to the latest version. Per the v1.10.0 release notes of @actions/core:

saveState and setOutput now use environment files if available


Notes:

  • I'm not sure if the contents of the dist/ directory should be updated as part of this Pull Request or not. If yes, I'm not quite sure how to do that as there's no npm script (or similar) for it as far as I'm aware... Feel free to push a commit to update dist/ if that's required 🙂
  • I kept @actions/core pinned (i.e. without leading ^) even though it's the only dependency that's pinned. I wasn't sure this is intended so I just kept it like it was.
  • The re-ordering of dependencies happened automatically when running the npm install command. Happy to revert that change if desired, but I believe it makes more sense to keep it this way as anyone running npm install will run into this change.

(ordering of dependencies happened automatically when running the
`npm install` command.)
@vhsantos26
Copy link

@zricethezav do we have any plan to have this merge and release?

CC: @ericcornelissen

@calmonr
Copy link

calmonr commented Nov 9, 2022

I will ping @weineran that I think can help us with that as well.

Thank you for the contribution @ericcornelissen.

@weineran
Copy link
Contributor

weineran commented Nov 9, 2022

Hi everyone, sorry for the delay on this one. @ericcornelissen Thanks for your contribution.

The reason for the delay is that this is the first PR we've received from the community since we released v2 under a commercial license. That means we had to be a little thoughtful about the legal implications of accepting community contributions. And thinking about legal matters isn't much fun so we procrastinated...

Anyway, we're hoping to provide some clarity on the path forward for community contributions later today or tomorrow, so stay tuned!

@weineran
Copy link
Contributor

weineran commented Nov 9, 2022

@ericcornelissen We just posted our contributing guidelines including a section called "Legal": https://github.com/gitleaks/gitleaks-action/blob/master/CONTRIBUTING.md#legal

Can you take a look and let us know if you're OK with that? If you have any questions/concerns, let us know.

@weineran
Copy link
Contributor

weineran commented Nov 9, 2022

Actually, here is a link to that file on the current commit, so this link will be static: https://github.com/gitleaks/gitleaks-action/blob/646a318983bffac15b4946496d999f7a99ae354c/CONTRIBUTING.md

@ericcornelissen
Copy link
Contributor Author

@ericcornelissen We just posted our contributing guidelines including a section called "Legal": [646a318/CONTRIBUTING.md]

Can you take a look and let us know if you're OK with that? If you have any questions/concerns, let us know.

I'm okay with that. I updated my branch so that the legal text you linked to is also present on my branch as a way of re-enforcing that.

@calmonr
Copy link

calmonr commented Nov 10, 2022

I'm wondering if it's necessary to build the action as well. I noticed that a few previous PR bumping dependencies also have the dist/index.js built.

cc @ericcornelissen @weineran

@weineran
Copy link
Contributor

I'm not sure if the contents of the dist/ directory should be updated as part of this Pull Request or not
I'm wondering if it's necessary to build the action as well

Yes indeed, @ericcornelissen can you try to build the dist per the new instructions in the quickstart section here?
https://github.com/gitleaks/gitleaks-action/blob/master/CONTRIBUTING.md#quickstart

Or if you'd rather hand this off to me, that's fine too. Let me know!

@ericcornelissen
Copy link
Contributor Author

ericcornelissen commented Nov 10, 2022

Yes indeed, [...] can you try to build [...]

Should be done with 4d1c233

[...] build the dist per the new instructions in the quickstart section here?

I managed, but I think the instructions can be improved. Following those steps won't always work because ncc isn't guaranteed to be available on the system after npm i (or nvm use for that matter) - @vercel/ncc isn't even a dependency of this project.

I'd be happy to provide more feedback/help improve the instructions in a separate thread from this 🙂

@weineran
Copy link
Contributor

I managed, but I think the instructions can be improved.

Yeah, sorry for the sparse instructions :/ Glad you were able to push through and get it to work.

I'd be happy to provide more feedback/help improve the instructions in a separate thread from this 🙂

Yes, feedback/help is always welcome!

@weineran
Copy link
Contributor

Noticing the checks are failing b/c they're not picking up the GITLEAKS_LICENSE secret that's stored in our organization. I need to look into that before I merge this although it's very likely unrelated...

@ericcornelissen
Copy link
Contributor Author

ericcornelissen commented Nov 10, 2022

Noticing the checks are failing b/c they're not picking up the GITLEAKS_LICENSE secret that's stored in our organization. I need to look into that before I merge this although it's very likely unrelated...

Note that secrets are not available when running in the context of a fork. Per the documentation for "Encrypted secrets":

Note: With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository.

You can also see this from the workflow logs. For a successful run it looks like:

  env:
    GITHUB_TOKEN: ***
    GITLEAKS_LICENSE: ***

whereas for a failed run of this Pull Request it looks like:

  env:
    GITHUB_TOKEN: ***
    GITLEAKS_LICENSE: 

@weineran
Copy link
Contributor

Makes sense. Thanks for the pointer to the docs.

I've created a new branch develop off of master. Can you edit your PR to use develop as the base branch? Then after we merge to develop, I can open a PR from develop to master and the github actions should run.

@ericcornelissen ericcornelissen changed the base branch from master to develop November 10, 2022 19:55
@ericcornelissen
Copy link
Contributor Author

Makes sense. Thanks for the pointer to the docs.

No problem 😄

I've created a new branch develop off of master. Can you edit your PR to use develop as the base branch? Then after we merge to develop, I can open a PR from develop to master and the github actions should run.

Done 👍

@weineran weineran merged commit 2693209 into gitleaks:develop Nov 11, 2022
@weineran
Copy link
Contributor

✅ Merged to master here: #103

@weineran
Copy link
Contributor

✅ After releasing master to v2, confirmed the warning is now gone: https://github.com/gitleaks/gitleaks-action/actions/runs/3442467251

Thanks @ericcornelissen !

@ericcornelissen ericcornelissen deleted the 100-bump-actions-core branch November 11, 2022 07:36
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.

Update set-output command
4 participants