-
Notifications
You must be signed in to change notification settings - Fork 14.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
fix(providers/amazon): handle missing LogUri in emr describe_cluster API response #31482
fix(providers/amazon): handle missing LogUri in emr describe_cluster API response #31482
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.
LGTM
If this returns none, what happens with the displayed link that gets generated from this value? |
171035e
to
552bf22
Compare
@ferruzzi Hi, I just tested it. It will link to a None bucket. |
Should I disable it if LogUri not found? |
Hi @potiuk , I encounter the following CI failure. But it doesn't seem to be something I introduce in this PR. Should I fix it in this PR? If so, could you kindly point out where this is from? Thanks!
|
@Lee-W I see a similar issue in my PR and have added a comment regarding this #31201 (comment) Apparently, @uranusjr did try to reproduce this with the PR #31489 as this was observed in |
552bf22
to
0bc3902
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.
LGTM
IMO this is not a requirement to merge this fix, but it's a nice enhancement for the AWS operators links which can be implemented separately. |
I'm not thrilled with the idea of knowingly vending a broken link. What about a compromise for now where if the link isn't available, return a string along the lines of "N/A"?I Love the idea of disabling the link if it's not available, but a static string message would be better than a dead link IMHO |
+1 to this idea and perhaps some more description in the string to include something like "you have disabled logging and hence ...." based upon the comment #31480 (comment) might hint the user why it is N/A, no? |
Hi @ferruzzi @pankajkoti , thanks for your suggestions! I'm not sure how I can add those descriptions but I disabled it if the link is not available. |
b316e20
to
a0ec623
Compare
I love it, thanks for doing that. I think pankajkoti's suggestion was around the phrasing where you put "No URL found for EMR Cluster logs". My initial suggestion was super vague and I think they were suggesting it be more descriptive. I think you have it covered, but I'll leave that to them. |
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 approve pending passing CI. I have seem that test fail on occasion for a timeout, try bumping it again.
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.
Should we add a test here for this? At least one with covering that if cluster_info does not contain LogUri, it does not raise an exception anymore?
Looks good to me otherwise.
LGTM, but missing test coverage. |
a0ec623
to
4d414b9
Compare
@pankajkoti Thanks for your suggestion! I just added it. |
According to the this document, it seems we might not always be able to get ["Cluster"]["LogUri"], and we encounter errors after the release in #31322. In this PR, I set the return value of
get_log_uri
toNone
if we cannot getLogUri
Closes: #31480
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.