-
Notifications
You must be signed in to change notification settings - Fork 0
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
Revert to old tacos unlock #197
base: main
Are you sure you want to change the base?
Conversation
.github/workflows/tacos_unlock.yml
Outdated
runs-on: ubuntu-latest | ||
permissions: | ||
contents: read | ||
pull-requests: write | ||
id-token: write | ||
|
||
env: | ||
TF_ROOT_MODULE: ${{matrix.tf-root-module}} |
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 still needed because the setup step breaks otherwise.
https://github.com/getsentry/tacos-gha.demo/actions/runs/8425696855/job/23079804120
if we really wanted to remove this, it would require also editing/reverting ./tacos-gha/.github/actions/setup
which every other workflow is using, so just leaving it like this is better.
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.
No I think leaving things in this state is causing the system to get into an inconsistent 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.
same comment as ever: there should be no matrix
.github/workflows/tacos_unlock.yml
Outdated
runs-on: ubuntu-latest | ||
permissions: | ||
contents: read | ||
pull-requests: write | ||
id-token: write | ||
|
||
env: | ||
TF_ROOT_MODULE: ${{matrix.tf-root-module}} |
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.
No I think leaving things in this state is causing the system to get into an inconsistent state.
lib/tf_lock/release.py
Outdated
for path in paths: | ||
tf_lock_release(path, env=environ.copy()) | ||
if tf_lock_release(path, env=environ.copy()) is None: |
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.
You're dropping the UserError on the ground if/when it's returned.
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.
It gets chalked up as a failure, I think the errors are written out and that's handled seperately, we can just have it return 1 or 0 and just add that?
lib/tacos/terraformers.py
Outdated
def convert_terraform_result( | ||
result: TerraformerResult, | ||
) -> Dict[str, Union[str, List[str]]]: | ||
"""Convert TerraformerResult to a JSON-serializable dictionary""" | ||
return { | ||
"GETSENTRY_SAC_OIDC": result.GETSENTRY_SAC_OIDC, | ||
"SUDO_GCP_SERVICE_ACCOUNT": result.SUDO_GCP_SERVICE_ACCOUNT, | ||
# Convert each OSPath in the set to a string, then convert the set to a list | ||
"slices": [str(path) for path in result.slices], | ||
} |
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.
If you change the definition of slices
to tuple[str]
then dataclasses.asdict
will do this for you.
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.
wouldn't slices then just be a tuple that can only hold one string?
4461ae4
to
12b868f
Compare
e5248ad
to
34bc6d9
Compare
# This is the 1st commit message: debugging # This is the commit message #2: debugging # This is the commit message #3: debugging # This is the commit message #4: proper json encoding # This is the commit message #5: convert list to json string # This is the commit message #6: trying this out instead of all slices # This is the commit message #7: pipe in all the slices first
remove lines_to_paths update success update failure
correct details for failure need the heading detail tag
add to both
trying gcp auth again spaces spaces debug gcp auth in the right place
34bc6d9
to
de52de6
Compare
This is the older version (no matrix, need to remove) to prevent tacos unlock from missing slices that were touched in the PR but later removed.
this is a successful run getsentry/tacos-gha.demo#2423 (comment)