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
Issue/8653 fix quickstart failures #141
Issue/8653 fix quickstart failures #141
Changes from all commits
0502806
2442ec8
d209614
0d31ffe
64993d4
2228169
338e59b
ebbef80
a43f622
3fb0435
c5f8fdf
dec3585
86b1da5
d7fda7a
75d0094
d616ff0
2cf773b
ce14f99
77e2640
3eef27b
6c04519
77fe8a1
9df5444
5fef7b9
49a048d
74e3771
a0c8155
f8fd8aa
19607f9
5248dd3
1a13c52
acd7ecc
fff15b9
1b1f3d5
2626de8
568aa51
820da0a
0ed5c7d
cd17924
3c22224
8cdfbdf
e385394
94a184c
f788e84
112da96
0998da5
1964e42
d10df94
d308b0a
773f3bd
624358a
217f7dc
bef1d80
bfdb574
c42124b
5a9e765
227b896
f57c3c9
4d1b2fb
70ace1e
4f0d32c
b61a268
f81c189
3dfebe2
d95f1ff
54e9745
6879b9b
72f5130
9514f0a
5e0beb1
81701dd
d1e3752
88618d0
601368d
650b6a7
2062d8f
64f2fed
b7e514b
ee0c4bd
31cc1c4
0499788
4b56cd3
d527eea
04f4ac7
341458f
402ae54
c8fb9dc
12f97eb
051719d
545d00c
0ac01c7
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.
I think it would be good to additionally request review from someone of the solutions team for this file.
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 you have anything specific in mind to pay attention to ?
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.
Just whether this is a good approach to validate that everything was deployed. But looking it over once more, I don't think it necessarily has to be someone from the solutions team. I would add a second reviewer, but rather for the full PR, not just this file.
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.
There is a race condition between the
export
command and thisresource_list()
command. The export command releases the version, but this happens asynchronously, so theresource_list()
might be executed before the version is released. This way we might be checking an outdated state.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 might be missing something but I think this is currently acceptable since the
done_deploying
check is performed inside aretry_limited
and theexpected_resources
parameter holds the version we want to check for ? ieThere 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.
Currently the
done_deploying
method does an assert on theexpected_resources
, which means that it raises an exception in case of failure. Theretry_limited
doesn't catch this exception so it will crash. If we change this assertion to an if-condition that returns False in case of a mismatch, I agree with the above-mentioned statement.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.
Oof nice catch
This file was deleted.