-
Notifications
You must be signed in to change notification settings - Fork 380
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
Gzip content cache and bugfix cache time calculation #790
Conversation
bbf6022
to
749d5af
Compare
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.
In general I think it makes sense that we ignore spec.json if we use something else like gzipJson or url. At the same time it's a breaking change since it will change functionality even though that way of working is probably "wrong".
About "Have to create a new content field in the status since changing a type from string to []byte would prevent the operator from reading existing status content which wouldn't be valid base64. Perhaps should remove the old one?"
Once again this makes a breaking change, at the same time this status filed is new and there are probably not many people that rely on it. So from that point of view I would say it's probably better to remove it to minimize confusion and problem in the future.
I can't remember what we said during the meeting how to handle this PR in general.
Any way I added a few comments.
a73ecee
to
b78014a
Compare
b78014a
to
35efe0e
Compare
From what I can tell it shouldn't be a problem to remove the old I changed the clearing of the old |
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.
LGTM!
Description
Three things happening here:
spec.json
content before setting thestatus.contentGzip
field to further down-size the CR. Not sure if this is a good idea. Might make some users unhappy if things disappear?Have to create a new content field in the status since changing a type from
string
to[]byte
would prevent the operator from reading existing status content which wouldn't be valid base64. Perhaps should remove the old one?Relevant issues/tickets
#789
Type of change
Checklist
Verification steps