-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
devops: automate bug report tests #9386
devops: automate bug report tests #9386
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a great start. Thanks. I've done a first pass of review comments. I have not tested this out yet. I guess I need to try your workflow out on a forked copy. Any chance you could look at these comments first and then I will give it a spin. Cheers
|
||
- name: Run tests | ||
id: run_tests | ||
env: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't pass this job any secrets.
There is nothing in npm run badge
to scrub or obfuscate credentials so any credentials we pass in are going to be echoed out in the request details. GH actions does have some built in masking functionality, but it won't cover basic auth credentials for example. They get base64 encoded. Obviously this means there are 9 service (out of ~200) where this will fail but I think we can live with that.
If we want to improve that, I would want to implement something at the logging layer that optionally prevents credentials from being included in the log output at all. Although we would want it to be an optional flag - outputting the credentials is really useful locally. I used it literally yesterday when I was working on #9387 to make sure the auth was being passed properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove the secrets (will commit soon), but i am not sure that only 9 services will be affected.
For example, the github token is there to avoid rate limit with a large number of services.
Regarding avoiding secrets from leaking, seems a bit risky. I think we could open a new issue to get ideas about how to tackle that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub is a "service"
GitHub Issues or GitHub License is a "badge"
So it is one "service", but your core point is completely right: GitHub is a service that has a lot of badges and they are really widely used so it would be very nice if that one worked.
Fortunately, if we're only calling one badge URL we don't really need a token. We need one under test because we're making lots of API calls. Anonymous usage would be fine here because we're not going to hit the late limit by making only one request. Allowing GH badges to work with no GitHub credentials at all would be a great solution. This has actually been a really long-standing issue affecting local development too: #2754 I don't think we need to fix that for this workflow to be useful, but if you were looking for a next issue to help with, #2754 would be a great shout.
We can merge this as it stands and treat that as a follow up.
- name: Setup | ||
uses: ./.github/actions/setup | ||
with: | ||
node-version: 16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the moment, this needs to stay as 16 due to strict-englines
, but I'm going to quickly note here that when we merge #9385 we will need to make this 18.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, whatever gets merged first should update the later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I'll update #9385 after I merge this
Devs run `npm run badge` often when getting a bug report for shields.io service. This PR automates this tests so the maintainer can get test results automaticly when getting into a new ticket. Add test-bug-run-badge.yml workflow for github actions automation. Will only run if bug has the 'qeustion' label added by bug reporting template. And will only setup enviorment and perform the test if the link provided in the issue is valid and the issue is related to shields.io. Link to job results is sent as a new comment on the issue. Resolves badges#9351
675fdb7
to
a84df7f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this. This was a slightly unusual but very welcome contribution. Most contributors who are new to the project focus on adding more badges. A PR adding automation that will help the core team to maintain the project more effectively more rare, but much appreciated.
Devs run
npm run badge
often when getting a bug report for shields.io service. This PR automates this tests so the maintainer can get test results automaticly when getting into a new ticket. Add test-bug-run-badge.yml workflow for github actions automation.Will only run if bug has the 'qeustion' label added by bug reporting template. And will only setup enviorment and perform the test if the link provided in the issue is valid and the issue is related to shields.io. Link to job results is sent as a new comment on the issue.
Resolves #9351
Image of the bot in action