cmd-cloud-prune: remove the unneeded exception #3889
Conversation
This exception was not supposed to be added there since that check was just added to make sure there isn't any build artifacts in meta.json toplevel keys that is in unsupported list. So far, we only prune aws and gcp. There are many top-level keys that are not artifacts-related so this check would throw the exception when not needed.
|
hmm. I'm not actually sure the get_supported() check is actually serving a purpose here. I think it makes more sense for cloud_images where we need to prune from the cloud platforms and not so much the images (artifact files) that we prune from s3 so it would make more sense to validate there are no cloud platform uploads that we don't know about when doing a cloud prune. The only problem there is that the cloud platform uploads aren't nested under a separate key in the meta.json (i.e. cloud-uploads or something). Thus we can't really tell if there are ones we don't know about because we don't know what keys at that level in the JSON are cloud uploads or not. I really think the only thing we can do is drop this check.. At best we could replace it with a check to verify that the list of images in images-keep are in the meta.json for that build? Even that I'm not sure on because I guess there could be builds where the build didn't get far enough to upload those images. |
|
Yes, this is intended for cloud images (i.e. only relevant for the cloud pruning step).
Right yeah, this is what the schema hash is intended to awkwardly guard against. The supported/unsupported lists are up to date as of this schema revision. If anyone changes the schema (e.g. adds a new platform support), then CI will fail unless they also update the hash in this file. It's definitely not great, but I wanted something for now at least until we improve the situation (e.g. tagging them in the schema somehow) so that if someone hacked on this and the three of us were offline when that happened, it wouldn't just slip by. |
Does that mean you agree we should remove it the check?
There is a CI check that validates the hash in the comment in this file? |
No, only that it's most relevant for the cloud pruning step. I.e. we can't add
Yeah, it's part of the other schema checks we do: Line 94 in ea78d55 |
|
To make it easier, I think we should change the variable name to something like Also @jlebon should we add |
The "azurestack", "digitalocean", and "exoscale" keys are all image artifacts that go under the `images` key. So they shouldn't be listed as optional entries in the top-level dict. Remove those until we actually have support for uploading to those clouds (though e.g. azurestack is clearly one to which that will never apply). See also: coreos#3889
|
Had a chat with @dustymabe and @gursewak1997 OOB. Dusty correctly noticed that some of those keys are not cloud resources and shouldn't be listed there. The source of truth for this is the schema which lists them as top-level keys. Fixed that in #3891. |
This exception was not supposed to be added there since that check was just added to make sure there isn't any build
artifacts in
meta.jsontop-level keys that is in unsupported list. So far, we only prune aws and gcp. There are many top-level keys that are not artifacts-related so this check would throw the exception when not needed.