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

Permissions readme #80

Merged
merged 7 commits into from
Jul 26, 2024
Merged

Permissions readme #80

merged 7 commits into from
Jul 26, 2024

Conversation

Achllle
Copy link
Contributor

@Achllle Achllle commented Jul 23, 2024

What type of PR is this? (check all applicable)

  • Documentation Update

Description

  • Adds the required permissions to the README for this to work at all.
  • Notes for those using forks

Notes to Reviewer

I've made some other minor improvements, including specifying the full version in the README's action.yml example. With the current v1, some of the features described in the same readme do not work. Happy to replace that with a note specifying people should consider using the latest version.

Link to issues addressed

Copy link
Member

@JavierCane JavierCane left a comment

Choose a reason for hiding this comment

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

Nice one @Achllle! Thanks a lot for the contribution 😊

I've given some feedback and suggested changes. Please review them when you get a chance 🙌

README.md Outdated
@@ -60,6 +64,8 @@ jobs:
files_to_ignore: ''
```

**note**: When using forks and where you don't want any PR to be able to execute code, replace `on: [pull_request]` with `on: [pull_request_target]` (see [GitHub docs](https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request_target)).
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion in order to use GitHub supported alerts:

Suggested change
**note**: When using forks and where you don't want any PR to be able to execute code, replace `on: [pull_request]` with `on: [pull_request_target]` (see [GitHub docs](https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request_target)).
> [!TIP]
> Replace `on: [pull_request]` with `on: [pull_request_target]` when using forks do not wanting any PR to be able to execute code ([more info](https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request_target)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should've seen this, thanks. Done manually since your suggestion contained a typo.

README.md Outdated
runs-on: ubuntu-latest
name: Label the PR size
steps:
- uses: codelytv/pr-size-labeler@v1
- uses: codelytv/pr-size-labeler@v1.10.0
Copy link
Member

@JavierCane JavierCane Jul 25, 2024

Choose a reason for hiding this comment

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

Suggested change
- uses: codelytv/pr-size-labeler@v1.10.0
- uses: codelytv/pr-size-labeler@v1

Reason why:

You stated that:

With the current v1, some of the features described in the same readme do not work

However, the problem is that we should have updated the v1 tag in order to point it to the most recent v1 release. That is, as far as we continue to respect Semantic Versioning, the new features introduced must not break backward compatibility. So in conclusion, I have changed the commit where the v1 points to in order to have the latest features available without having to specify a particular version.

  • Before:
    2024_07_25-15_15_24
  • After:
    2024_07_25-15_33_35

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'll revert my commit.

@Achllle Achllle requested a review from JavierCane July 25, 2024 19:36
Copy link
Member

@JavierCane JavierCane left a comment

Choose a reason for hiding this comment

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

Thanks for reviewing the suggestions. Merging! 😊

@JavierCane JavierCane merged commit f8701c0 into CodelyTV:main Jul 26, 2024
2 checks passed
@Achllle Achllle deleted the permissions_readme branch July 26, 2024 16:17
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.

Document required permissions Question: Does this action work with pull requests from forks?
2 participants