This repository was archived by the owner on Feb 6, 2025. It is now read-only.
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.
cri: handle the migration to the new configuration format #1251
cri: handle the migration to the new configuration format #1251
Changes from all commits
6bbe87f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why do we remove the CriConfDir ? It means if the customer has custom files, they will be removed, no?
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.
Not exactly.
clusterinit.WriteScaffoldFile(file, cfg)
will write scaffold files into that directory. Thus, any files with the same names will be effectively replaced. This means that customers have to back things up before performing the migration (as in with any migration).That being said, if, for whatever reason, we fail to write these files, we should remove everything instead of sticking into an intermediary result (e.g. imagine that one file was successfully written but the other didn't). We don't want these kinds of scenarios, it's all or nothing.
Therefore, I think that this is actually the safest approach.
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.
I think in this case, the files being replaced are the "caasp defaults", which is fine, and the customer shouldn't touch them, so we are good. Yes, it's indeed safer to backup, while not necessary.
That's what I am not getting my head around. For me, we should fail, and say "There is something wrong that happened here, maybe you can figure it out with this error: %err". Not deleting the whole folder, which might contain user configuration on top of our default configs. That's very disruptive to me.
I agree it's better to bail, but I don't think it's better to delete the folder.
Do you mean that, because this migration only runs ONCE, it should be okay to delete the folder, as there is no user configuration yet, and that code will probably never be used for something else? I am fine with that, but I would prefer to add this explicitly in the comments. A comment just before the dir removal should be enough, to clarify that "this is okay to do so because this process only runs once, so there should be no failure, and there should be no user configuration at this point yet"