-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add separate error message for not supported operation on min_cpu_platform #11289
base: main
Are you sure you want to change the base?
Add separate error message for not supported operation on min_cpu_platform #11289
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @melinath, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
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.
Rather than clarifying the error message, would it make sense / be reasonable / solve the problem to add a section like:
if d.HasChange("min_cpu_platform") && d.HasChange("machine_type") {
// set machine type to AUTOMATIC - it will be set to the actual value later.
}
Tests analyticsTotal tests: 976 Click here to see the affected service packages
View the build log |
Both parameters should be changed in one API call to if d.HasChange("min_cpu_platform") || d.HasChange("machine_type") {
// set up Instance resource with updated configs and send an Instances.Update call
} Btw. the general pattern of issuing individual fields modification calls to the API seems to be more driven by historic evolution of the code than through an actual benefit. It should be refactored to construct a single API call for |
yeah, |
@khajduczenia it sounds like you're saying that a single call to Update should be able to change them simultaneously, though? |
The |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 976 Click here to see the affected service packages
Action takenFound 13 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
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.
These test failures look legitimate.
-
TestAccComputeInstance_diskEncryptionRestart: Looks like the Update call needs to include the encryption key (if one is set)
-
The rest are failing with errors like:
Error: googleapi: Error 400: Invalid value for field 'resource.fingerprint': '<ByteString@e19b8e1 size=8 contents="\363\377@36\357\306u">'. Fingerprint mismatch, some instance properties may have changed or you are using a different API version to get the instance resource., invalid
It looks like the StopInstance operation probably changes the fingerprint, so it would need to be re-fetched prior to calling Update.
Fixed the fingerprint errors by using the tpg_resource.Retry() block The encryption keys are a bigger issue. The Fails with this error even when the key is provided
Gonna experiment whith this for a bit, to see if it's an API bug or not I suppose there will be a refactor on the whole |
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.
Removing from review queue pending those changes
The API doesn't read provided raw keys to decrypt the disks Here is a correct instance payload that gets rejected in the test
Test log from this API call
It actually doesn't matter what disk is in the error code because any disk having a CSEK will cause a fail on this API call |
Thanks! Any chance you have a reference link for the API bug? |
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.
Removing from my review queue for now - let me know when you want me to take a look
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 980 Click here to see the affected service packages
Action takenFound 15 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
I don't think these 2 test fails are related because they pass in my env that's a few commits behind |
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.
removing from my review queue pending API fix
@karolgorc, this PR is waiting for action from you. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. If no action is taken, this PR will be closed in 28 days. This notification can be disabled with the |
@karolgorc, this PR is waiting for action from you. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. If no action is taken, this PR will be closed in 14 days. This notification can be disabled with the |
@karolgorc please see this comment: #11742 (comment) If you would like assistance converting files, we need your permission to push changes to this branch. |
@karolgorc, this PR is waiting for action from you. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. If no action is taken, this PR will be closed in 2 weekdays. This notification can be disabled with the |
Removing automatic closure to give time to convert to Go & because it looks like this is pending an API fix |
add fingerprint fixes, add fetching raw keys before API call change functions used to change min_cpu_platform and machine_type and move them to the same condition Add separate error message for not supported operation on min_cpu_platform parameter
30ec95e
to
33a8d24
Compare
There was a merge conflict as I was converting this from Ruby to Go, not sure if I resolved it correctly. The backup branch is here. Please double-check that the logic is still correct. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 1018 Click here to see the affected service packages
Action takenFound 13 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
This test error is currently expected and waiting for an API fix on instances.Update
|
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.
Removing from my queue pending the API fix.
Resolves issue #15471
Refactor how machine_type and min_cpu_platform are updated
Release Note Template for Downstream PRs (will be copied)