-
Notifications
You must be signed in to change notification settings - Fork 254
[QE] fix windows pr check failure #4706
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
Conversation
Reviewer's GuideEnhanced Windows QE GitHub Actions workflow by updating the Mapt CLI image, increasing VM CPU allocation, and expanding the list of Azure regions excluded for spot instances. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @lilyLuLiu - I've reviewed your changes - here's some feedback:
Overall Comments:
- It might be helpful to add a comment explaining why these specific regions are excluded from spot instances.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
windows pr check failure is also caused by redhat-developer/mapt#456 |
Can you add some explanations in the commit log or the PR description explaining why we need to add more CPUs and ignore another region? |
The windows PR failed because the reserved machine cup is not enough to start a VM. The mapt default number 8 is not working as expect (issue redhat-developer/mapt#456). Hold this pr and wait this mapt issue get fixed. |
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.
Hey @lilyLuLiu - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adrianriobo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test security |
@lilyLuLiu: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
@cfergeau Could you help check the failed check of ci/prow/security? |
snyk complains about this:
We recently made this developer password configurable, this would be a consequence of this change. These default values are intentional, @albfan took care of white listing similar issues with snyk a while ago. |
/test security |
I have ignored those complain because this is not a security concern (developer password is default one). |
Description
Fixes: #N
Relates to: #N, PR #N, ...
Type of change
test, version modification, documentation, etc.)
Proposed changes
Testing
Contribution Checklist
Summary by Sourcery
Update Windows QE GitHub Actions workflow to fix Windows PR check failures by upgrading the Azure provisioning tool, increasing resource allocation, and adjusting region exclusions
CI: