-
Notifications
You must be signed in to change notification settings - Fork 38
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 sky key secret creation so it won't break release testing locally #1594
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This stack of pull requests is managed by Graphite. Learn more about stacking. |
BelSasha
changed the title
fix sky key secret creation so it won't break release testing locally
[WIP] fix sky key secret creation so it won't break release testing locally
Dec 15, 2024
BelSasha
force-pushed
the
sb/fix-sky-key-issue-for-tests-den-launcher
branch
from
December 15, 2024 13:44
9599b77
to
7f6835a
Compare
This was referenced Dec 15, 2024
BelSasha
force-pushed
the
sb/fix-sky-key-issue-for-tests-den-launcher
branch
from
December 15, 2024 14:21
7f6835a
to
5674950
Compare
jlewitt1
reviewed
Dec 15, 2024
@@ -107,7 +107,15 @@ def supported_providers(): | |||
|
|||
@classmethod | |||
def sky_secret(cls): | |||
from tests.fixtures.resource_fixtures import create_folder_path | |||
|
|||
secrets_name = "ssh-sky-key" |
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.
instead of changing logic to accomodate the test, we can save "ssh-sky-key" as a constant, and then just overwrite it in the test
rh.constants.SSH_SKY_KEY = ""
BelSasha
force-pushed
the
sb/fix-sky-key-issue-for-tests-den-launcher
branch
2 times, most recently
from
December 15, 2024 14:49
e581892
to
87832ad
Compare
BelSasha
force-pushed
the
sb/fix-sky-key-issue-for-tests-den-launcher
branch
from
December 15, 2024 14:54
87832ad
to
79832f6
Compare
BelSasha
changed the title
[WIP] fix sky key secret creation so it won't break release testing locally
fix sky key secret creation so it won't break release testing locally
Dec 15, 2024
jlewitt1
approved these changes
Dec 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The problem:
ssh-sky-key
is not being deleted during den clean up (which makes sense, it should not have). Therefore, there might be a mismatch between thessh-sky-key
values saved as a den secret, and the localssh-sky-key
values. (For example thessh-sky-key
values saved in den might belong to user A who created and saved this secret forden_tester
and the local values might belong to user B who is running release testing withden_tester
). This is the explanation to the following issue I was seeing:The fix probably should be creating a new
ssh-sky-key
each time we are creating a cluster using the test cluster fixtures. This way we could parallelly run tests using den_tester (CI, few runhouse members running release tests locally etc), without getting the above error, since now the values of the local sky key and the one saved in den will match.