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

Remove pull request template #18104

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

aeisenberg
Copy link
Contributor

Replace with an updated check-change-note.yml workflow.

Add a comment whenever a pull request has changes that may need to be tested in autofix. Also, remove parts of the checklist that are not related to autofix.

Replace with an updated check-change-note.yml workflow.

Add a comment whenever a pull request has changes that may need to
be tested in autofix. Also, remove parts of the checklist that are
not related to autofix.
@aeisenberg aeisenberg requested a review from a team as a code owner November 25, 2024 20:53
@aeisenberg aeisenberg added the no-change-note-required This PR does not need a change note label Nov 25, 2024
@aeisenberg
Copy link
Contributor Author

The new workflow job won't run until this PR is merged since it runs under pull_request_target.

- "!**/experimental/**"
- "!ql/**"
- "!rust/**"
- ".github/workflows/check-change-note.yml"

jobs:
add-pr-reminders:
env:
REPO: ${{ github.repository }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
REPO: ${{ github.repository }}

https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables#default-environment-variables
explains GITHUB_REPOSITORY as

The owner and repository name. For example, octocat/Hello-World.

So we can skip declaring our own variable here.

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 was just following along with REPO elsewhere in the file. Looking in history, I see that REPO was added 13 months ago...by you. 😄

https://github.com/github/codeql/pull/14542/files#diff-b2dac1c5458a76f5ad8257a642aeac9ecf2c1b44ee9ca36b62f4d49d8ff24f05R19

Is there a reason to change it in this job, but not the other?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have pushed a commit that makes use of GITHUB_REPOSITORY everywhere in this file.

.github/workflows/check-change-note-and-reminders.yml Outdated Show resolved Hide resolved
.github/workflows/check-change-note-and-reminders.yml Outdated Show resolved Hide resolved
@@ -0,0 +1,7 @@
### Pull Request reminders for autofix
Copy link
Contributor

@aschackmull aschackmull Nov 26, 2024

Choose a reason for hiding this comment

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

Can we please restrict this comment to only occur when there are changes in */ql/src/**? Otherwise I think we'll add too much noise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking back: didn't we actually decide against this commenting approach in the first place in #17017 (comment)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, upon further thought, I don't think we should spam PRs with a comment like this.

@aeisenberg
Copy link
Contributor Author

I just had a chat with @oscarsj about this. In the next few weeks, his team will be looking into how to make the autofix testing tools easier to use. I'll keep this PR open for now and we can discuss later. Thanks for your feedback so far.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants