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: separate knip-report.yml and deploy.yml comments #337

Merged
merged 8 commits into from
Oct 16, 2024

Conversation

zugdev
Copy link
Contributor

@zugdev zugdev commented Oct 14, 2024

Resolves #298

@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented Oct 14, 2024

Copy link
Contributor

github-actions bot commented Oct 14, 2024

Preview Deployment
93e6dd89507b4c74d17c31d98abde3e0ecbf315c

Copy link
Contributor

github-actions bot commented Oct 14, 2024

Unused dependencies (2)

Filename dependencies
package.json lodash.capitalize
react

@zugdev zugdev marked this pull request as ready for review October 14, 2024 22:00
@zugdev zugdev requested a review from Keyrxng as a code owner October 14, 2024 22:00
@zugdev
Copy link
Contributor Author

zugdev commented Oct 14, 2024

As you can see here this works. But I don't know what bot I should auth, please let me know because at least apparently only github-actions bot is reporting Knip.

@0x4007
Copy link
Member

0x4007 commented Oct 14, 2024

github-actions bot is fine to auth with. Is it using your code within this pull?

@zugdev
Copy link
Contributor Author

zugdev commented Oct 14, 2024

It isn't apparently :p

Will run QA

@zugdev
Copy link
Contributor Author

zugdev commented Oct 15, 2024

@0x4007 QA here: zugdev#1 you can see through edits history, separated comments were respected.

@rndquu rndquu self-requested a review October 15, 2024 08:41
@rndquu
Copy link
Member

rndquu commented Oct 15, 2024

@zugdev

  1. What is the root cause of the issue? Here the deploy.yml workflow tries to find a comment by comment author and here knip-reporter.yml posts a new comment providing a comment id. Why in some cases those comment ids are the same?
  2. Why do we need these steps? (one, two)

@zugdev
Copy link
Contributor Author

zugdev commented Oct 15, 2024

@zugdev

  1. What is the root cause of the issue? Here the deploy.yml workflow tries to find a comment by comment author and here knip-reporter.yml posts a new comment providing a comment id. Why in some cases those comment ids are the same?

The reason is knip-reporter.yml runs faster than deploy.yml, and since deploy.yml only looks for an existing comment with "github-actions[bot]" as poster's name, it will find the comment posted at knip-reporter.yml.

  1. Why do we need these steps? (one, two)

These steps ensure that the bot running those workflows are the user github-actions[bot], so it is the only one commenting in PRs. It's an authentication method required on issue's scope.

The solution summarized is to first check if who is running is github-actions[bot], if it is:

  1. knip-reporter.yml will look for a comment that starts with " | Knip Report | " , if not it will create a new one.
  2. deploy.yml will look for a comment with " | Preview Deployment | ", if not it will create a new one.

.github/workflows/deploy.yml Show resolved Hide resolved
@zugdev
Copy link
Contributor Author

zugdev commented Oct 15, 2024

Am I the one to solve conflict?

@0x4007 0x4007 merged commit 93e6dd8 into ubiquity:development Oct 16, 2024
3 checks passed
@rndquu
Copy link
Member

rndquu commented Oct 16, 2024

@zugdev

  1. What is the root cause of the issue? Here the deploy.yml workflow tries to find a comment by comment author and here knip-reporter.yml posts a new comment providing a comment id. Why in some cases those comment ids are the same?

The reason is knip-reporter.yml runs faster than deploy.yml, and since deploy.yml only looks for an existing comment with "github-actions[bot]" as poster's name, it will find the comment posted at knip-reporter.yml.

  1. Why do we need these steps? (one, two)

These steps ensure that the bot running those workflows are the user github-actions[bot], so it is the only one commenting in PRs. It's an authentication method required on issue's scope.

The solution summarized is to first check if who is running is github-actions[bot], if it is:

  1. knip-reporter.yml will look for a comment that starts with " | Knip Report | " , if not it will create a new one.
  2. deploy.yml will look for a comment with " | Preview Deployment | ", if not it will create a new one.

More questions:

  1. Why do we even need to modify knip-reporter.yml if deploy.yml now searches by comment body?
  2. I still don't understand why this step is required. As far as I understand both knip-reporter and deploy.yml will always run by github-actions[bot] (i.e. RUNNER_NAME = "Github Actions N"). Why don't we need this step for all our other workflows?

The changes in knip-reporter.yml and deploy.yml should be reflected in our template repo which is a base repository setup of all of the ubiquity repos so all changes must 100% reasonable because those changes will be reflected in all of our repos.

@rndquu
Copy link
Member

rndquu commented Oct 16, 2024

Am I the one to solve conflict?

Yes, PR authors are supposed to resolve code conflicts

@zugdev
Copy link
Contributor Author

zugdev commented Oct 16, 2024

  1. Why do we even need to modify knip-reporter.yml if deploy.yml now searches by comment body?

That's a good question. I chose this approach because we guarantee we are modifying the comment from github-actions[bot] and the specific workflow one. I am not exactly sure how the workflows in Ubiquity's bots are set since they are private, but if you had a ubiquity-knip-reporter[bot] and you just used the same comment ID approach: "${{ github.workflow }}-reporter" they would overwrite one another's comment. If that's not the case and won't ever be, or you find this solution horrible, I am comfortable switching to one where we just set different comment IDs. We would just need to make sure different bots have different comment IDs and we could achieve that by setting the comment ID at deploy.yml here as "${{ github.workflow }}-actions-bot-deployer" and in ubiquity-os-deployer[bot] to "${{ github.workflow }}-ubiquity-os-bot-deployer".

  1. I still don't understand why this step is required. As far as I understand both knip-reporter and deploy.yml will always run by github-actions[bot] (i.e. RUNNER_NAME = "Github Actions N"). Why don't we need this step for all our other workflows?

Part of the answer to this is above but it's also specificied in the issue's scope, and in this comment, that we should auth a bot to run these workflows.

@ubiquity-os ubiquity-os bot mentioned this pull request Oct 17, 2024
@rndquu
Copy link
Member

rndquu commented Oct 18, 2024

  1. Why do we even need to modify knip-reporter.yml if deploy.yml now searches by comment body?

That's a good question. I chose this approach because we guarantee we are modifying the comment from github-actions[bot] and the specific workflow one. I am not exactly sure how the workflows in Ubiquity's bots are set since they are private, but if you had a ubiquity-knip-reporter[bot] and you just used the same comment ID approach: "${{ github.workflow }}-reporter" they would overwrite one another's comment. If that's not the case and won't ever be, or you find this solution horrible, I am comfortable switching to one where we just set different comment IDs. We would just need to make sure different bots have different comment IDs and we could achieve that by setting the comment ID at deploy.yml here as "${{ github.workflow }}-actions-bot-deployer" and in ubiquity-os-deployer[bot] to "${{ github.workflow }}-ubiquity-os-bot-deployer".

  1. I still don't understand why this step is required. As far as I understand both knip-reporter and deploy.yml will always run by github-actions[bot] (i.e. RUNNER_NAME = "Github Actions N"). Why don't we need this step for all our other workflows?

Part of the answer to this is above but it's also specificied in the issue's scope, and in this comment, that we should auth a bot to run these workflows.

ok, since this step (preview URL with a claimable permit) is not required for https://github.com/ubiquity/ts-template then we don't really need to update anything in https://github.com/ubiquity/ts-template and may keep these "custom" deploy.yml and knip-reporter.yml solely in pay.ubq.fi

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.

Preview Deployer Bug
3 participants