-
Notifications
You must be signed in to change notification settings - Fork 167
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
cmd-cloud-prune: Fixed cloud pruning and backed up builds.json
#3863
Conversation
b51d001
to
9099834
Compare
9099834
to
03fef9a
Compare
src/cmd-cloud-prune
Outdated
build.setdefault("policy-cleanup", []).append("cloud-uploads") | ||
builds_json_data["builds"][index] = build | ||
build.setdefault("policy-cleanup", []).append("cloud-uploads") | ||
builds[len(builds) - 1 - index] = build |
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.
Hmm, that shouldn't be necessary. build
is the original object within builds
already.
src/cmd-cloud-prune
Outdated
# Save the updated builds.json to local builds/builds.json | ||
save_builds_json(builds_json_data) | ||
# Ensure the builds are ordered from newest to oldest. | ||
builds.sort(key=lambda x: x['id'], reverse=True) |
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.
We shouldn't be doing any sorting at all here. Why do we need this? How is the array getting unsorted in the first place?
# Ensure the builds are ordered from newest to oldest. | ||
builds.sort(key=lambda x: x['id'], reverse=True) | ||
# Save the updated builds.json to local builds/builds.json | ||
save_builds_json(builds_json_data, BUILDFILES['list']) |
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.
Looks like we lost the conditional on args.dry_run
here.
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 we want to see/print the updated builds.json
, we will need to skip on the args.dry_run
at couple of places in order to update the builds.json
.
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.
Hmm, right. But I think it's also weird to have a --dry-run
operation actually overwrite builds.json
. But OK, I think that's fine.
# Make sure we have the merged json as local builds/builds.json | ||
save_builds_json(remote_builds_json) | ||
# Make sure we have the merged json as local builds/builds.json | ||
save_builds_json(remote_builds_json, BUILDFILES['list']) |
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.
Lost conditional on dry run.
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 as above.
Can you expand on what went wrong that caused some of the build entries to get lost? |
03fef9a
to
25e0514
Compare
# Ensure the builds are ordered from newest to oldest. | ||
builds.sort(key=lambda x: x['id'], reverse=True) | ||
# Save the updated builds.json to local builds/builds.json | ||
save_builds_json(builds_json_data, BUILDFILES['list']) |
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.
Hmm, right. But I think it's also weird to have a --dry-run
operation actually overwrite builds.json
. But OK, I think that's fine.
src/cmd-cloud-prune
Outdated
with open(BUILDFILES['list'], 'r') as file: | ||
data = json.load(file) | ||
print("This is the updated builds.json\n") | ||
print(json.dumps(data, indent=2)) |
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.
print(json.dumps(data, indent=2)) | |
print(json.dumps(data, indent=4)) |
src/cmd-cloud-prune
Outdated
print("This is the updated builds.json\n") | ||
print(json.dumps(data, indent=2)) |
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.
How about something like:
print("This is the updated builds.json\n") | |
print(json.dumps(data, indent=2)) | |
print("---") | |
print(json.dumps(data, indent=4)) | |
print("---") |
?
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.
Yep, this should work.
Updated the process to ensure that the build order is preserved while keeping all existing builds from the builds.json file intact. Additionally, added a step to print the updated builds.json during a dry run, which will be uploaded in non-dry run mode. Also, the previous version of builds.json is now backed up to the S3 bucket as builds.json.bak.
25e0514
to
2338512
Compare
Updated the process to ensure that the build order is preserved while keeping all existing builds from the
builds.json
file intact. Previously, we were updating the builds on the wrong index after we switched the enumerating of the builds in reversed order which caused us to lose the builds and also switched the ordering.Additionally, added a step to print the updated
builds.json
during a dry run, which will be uploaded in non-dry run mode. Also, the previous version ofbuilds.json
is now backed up to the S3 bucket asbuilds.json.bak
.