Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Added the default ignore config for service disable error #476
Added the default ignore config for service disable error #476
Changes from 2 commits
e1276f6
c0187e0
b3c186a
ca35dd0
fb92d9c
ab5cc67
944ce9f
f2eac5d
48da7f7
3a7c37f
b6a1e4e
b775a5f
de603d6
aecb618
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@ParthaI Is there a more exact regex (using
^
,$
) we should add as an example here, in particular for the "API has not been used" case? We can probably removeother regex
as it's not helpful.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 is a really good addition to handle cases where, google uses the same 403 error code when - API are not enabled and when Quota exceeded. An exact regex mapping example to an error message would sure be helpful.
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 reference, this is reported as this:
So I think the current example with
API has not been used
would be enough.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.
@cbruno10, I have added the example for the exact regex (using ^, $) i.e
ignore_error_messages = ["^.*API\\s*has\\s*not\\s*been\\s*used.*$"]
. Also we can pass a message just like a string format, in both of the case the code changes are working fine.NOTE: In the .spc file, we should use
\\
instead of\
for the regex pattern to avoid errors related to escape characters. Otherwise, parsing the .spc file might result in an error like "Invalid escape sequence: The symbol "d" is not a valid escape sequence selector."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.
Does GCP ever send messages that don't have the
Message
property? If so, does this code still handle everything ok?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 haven't encountered any errors without a message property. Additionally, we are converting errors of type
googleapi.Error
, which always includes a message property. I've tested it multiple times with various connection combinations, and it's working correctly, handling errors as expected.If an error does not contain a
message
property, the code block above will not function as it is designed to handle errors based on their error message. In cases where themessage
property is not available, an alternative approach is to configure theignore_error_codes
in the .spc file. This will allow the following line of code to handle the error.According to the GCP API design documentation, errors include a message property. However, it's important to note that
Error messages are not part of the API surface. They are subject to changes without notice. Application code should not rely heavily on error messages as they may change.
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.
Does this function support matching on full regular expressions? How is this different than the regex
Match
function?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.
Yes, this function support matching on full regular expressions.
*func (re Regexp) FindString(s string) string
FindString returns a string holding the text of the leftmost match in s of the regular expression. If there is no match, the return value is an empty string, but it will also be empty if the regular expression successfully matches an empty string. Use FindStringIndex or FindStringSubmatch if it is necessary to distinguish these cases.
*func (re Regexp) Match(b []byte) bool
Match reports whether the byte slice b contains any match of the regular expression re.
IMO, it would be better to use
MatchString
function here. I can see in most of the place we useMatchString
function to validate patterns. https://github.com/search?q=repo%3Aturbot%2Fsteampipe%20MatchString&type=codeI have replace the function
FindString
toMatchString
.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.
Do we need this block still?
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.
Removed this block of code as we do not need those.