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

Enable labeler workflow #1286

Merged
merged 3 commits into from
Sep 15, 2022
Merged

Enable labeler workflow #1286

merged 3 commits into from
Sep 15, 2022

Conversation

seokho-son
Copy link
Collaborator

Signed-off-by: Seokho Son shsongist@gmail.com

Describe your changes

  • This PR is to update labeler workflow (automate labeling PRs)
  • The labeler workflow has been disabled to check a security issue within the labeler workflow. (a high level permission given to the pull_request_target option)
  • According to some valuable discussions with @yunkon-kim , we concluded that the concern with the permission issue can be minimized. (please review and discuss if there is any other concern)

Related issue number or link (ex: resolves #issue-number)

  • NA

Checklist before opening this PR (put x in the checkboxes)

  • This PR does not contain plagiarism
    • don’t copy other people’s work unless you are quoting and contributing it to them.
  • I have signed off on all commits
    • signing off (ex: git commit -s) is to affirm that commits comply DCO.

Signed-off-by: Seokho Son <shsongist@gmail.com>
@netlify
Copy link

netlify bot commented Sep 2, 2022

Deploy Preview for cncfglossary ready!

Name Link
🔨 Latest commit 0125572
🔍 Latest deploy log https://app.netlify.com/sites/cncfglossary/deploys/63208df2e14798000845632b
😎 Deploy Preview https://deploy-preview-1286--cncfglossary.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@seokho-son
Copy link
Collaborator Author

PTAL @yunkon-kim

Copy link
Collaborator

@yunkon-kim yunkon-kim left a comment

Choose a reason for hiding this comment

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

@seokho-son I have a simple suggestion as follows :-)

.github/workflows/labeler.yml Outdated Show resolved Hide resolved
- 'content/**'
branches:
- '**'
- pull_request_target
Copy link
Collaborator

Choose a reason for hiding this comment

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

Amending to @yunkon-kim's suggestion:

Suggested change
- pull_request_target
pull_request_target:
types: [opened, synchronize]
paths:
- 'content/**.md'
branches:
- 'dev-**'
- 'main'

If we add 'main' as target branch, then this will automatically label lang/en for the PRs that add/modify 'content/en/**.md' and target main!

(Ref: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#running-your-workflow-based-on-the-head-or-base-branch-of-a-pull-request-1)


For information:
The default value of sync-labels is false. (https://github.com/actions/labeler#inputs)
We might need to add action.yml to this repo and set the value of sync-labels to true,
to make synchronize-related thing work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jihoon-seo thanks for your updates!

I agree that main is a vital point in this workflow.

For a simple discussion:
I was thinking about the synchronize type. However, I couldn't find any cases of reassigning the label when synchronizing.

Also, when we add the synchronize type, this workflow will be triggered by every PR synchronization.

Therefore, it would be nice to add the 'synchronize' as needed.

If anyone has a case that synchronize is necessary, please feel free to share.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed "synchronize" for this time. Let's see it is necessary after some period. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@seokho-son

main was not added to the branches section. Is it ok?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yunkon-kim Added :)

@jihoon-seo jihoon-seo added the maintainers Use this label if PR requires maintainers to take action label Sep 6, 2022
Co-authored-by: Yunkon Kim  <7975459+yunkon-kim@users.noreply.github.com>
Signed-off-by: Seokho Son <shsongist@gmail.com>
Signed-off-by: Seokho Son <shsongist@gmail.com>
Copy link
Collaborator

@jihoon-seo jihoon-seo left a comment

Choose a reason for hiding this comment

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

LGTM!

@seokho-son
Copy link
Collaborator Author

@iamNoah1 I think this PR is ready :)

Copy link
Collaborator

@iamNoah1 iamNoah1 left a comment

Choose a reason for hiding this comment

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

lgtm

@iamNoah1 iamNoah1 merged commit 54f6ab0 into cncf:main Sep 15, 2022
@yunkon-kim
Copy link
Collaborator

@seokho-son
I think we should enable the "pull Request Labeler" workflow =)

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainers Use this label if PR requires maintainers to take action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants