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

feat: switch to tagless workflow success check #2

Merged
merged 9 commits into from
Jul 19, 2021

Conversation

meeroslav
Copy link
Collaborator

No description provided.

@meeroslav meeroslav marked this pull request as ready for review July 16, 2021 16:13
@meeroslav
Copy link
Collaborator Author

@JamesHenry Please review

One thing I would add - Mark current commit as v1 so that we can reference it in the readme of v2.

action.yml Show resolved Hide resolved
@meeroslav meeroslav requested a review from JamesHenry July 19, 2021 12:34
README.md Outdated
- [Background](#background)
- [License](#license)

> This documentation is for version 2.x.x. If you are using version 1.x.x you also need to include accomplanying [nx-tag-successful-ci-run](https://github.com/nrwl/nx-tag-successful-ci-run) as the last step of your job, since version 1.x.x depends on the existance of git tags that mark successful runs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are a couple of typos in here (accomplanying, existance) so it needs to be changed anyway, so may I suggest instead of this paragraph we instead simply link to the README.md at the latest v1 tag?

README.md Outdated
# Use ${{ secrets.GITHUB_TOKEN }} as a value
#
# Required: true
GITHUB_TOKEN: ''
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should use ${{ github.token }} rather than forcing the user to set up a secret in their repo and then reference that in the env.

One of the great things about github actions is the fact you get a token provisioned automatically for you

Copy link
Collaborator

Choose a reason for hiding this comment

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

See examples on the official org of actions: https://github.com/actions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

User does not need to set up new secret in their repo, the GITHUB_TOKEN exists by default. GITHUB_XYZ and github.xyz are the same. For some reason, it wasn't working when I tried to reference the env value in the action.yml without specifying it in the workflow. But works fine with GitHub context. Thanks!

https://github.com/octokit/action.js/blob/master/README.md

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated

- run: |
echo "BASE: ${{ steps.setSHAs.outputs.base }}"
echo "HEAD: ${{ steps.setSHAs.outputs.head }}"

# ===========================================================================
# OPTION 3) Specify all parameters
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not really what was intended by OPTIONs here.

There are two fundamentally different ways of using the SHAs - via environment variables, or via github job outputs.

This is not a third OPTION when it comes to fundamental usage of the SHAS, so please remove.

The configuration options section below was intended to exhaustively cover all the options available

@meeroslav meeroslav requested a review from JamesHenry July 19, 2021 14:29
README.md Outdated
- [Background](#background)
- [License](#license)

> This documentation is for version 2.x.x. If can find documentation for version 1.x.x [here](https://github.com/nrwl/nx-set-shas/blob/c8f5a54f6ee7f2127f3df063f36a0242faee4cb7/README.md).
Copy link
Collaborator

Choose a reason for hiding this comment

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

😬 typo again "if can find"

@meeroslav meeroslav requested a review from JamesHenry July 19, 2021 14:43
@JamesHenry JamesHenry merged commit 2eec050 into nrwl:main Jul 19, 2021
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.

2 participants