Skip to content
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

Fix bug with FBC YAML Operators #655

Merged
merged 3 commits into from
Apr 17, 2024
Merged

Fix bug with FBC YAML Operators #655

merged 3 commits into from
Apr 17, 2024

Conversation

JAVGan
Copy link
Contributor

@JAVGan JAVGan commented Apr 4, 2024

When an operator has catalog in yaml insted of json, some requests (like add) fails on "duplicate package" as merge_catalogs_dirs function generates FBC from hidden index.db created as json files and then copy over its content to final location where are all yaml files.

To prevent this issue this simple fix will walk through the config directory and convert any YAML files into JSON to ensure that everything uses the same format in the index image.

Refers to CLOUDDST-21432

@JAVGan
Copy link
Contributor Author

JAVGan commented Apr 4, 2024

@release-engineering/exd-guild-hello-operator PTAL.

I took the liberty to convert all files in the config dir to JSON as it's the default one from opm, having the solution "supporting" YAML in input but always outputting in JSON. Please let me know whether it's OK to go with this.

@lipoja
Copy link
Contributor

lipoja commented Apr 4, 2024

@JAVGan I have to think about this solution - tomorrow.
My assumption was that the error was caused by opm migrate where we we generated json but copied it over to /config dir so it ended up with two files (yaml, json) for one operator and then everything crashed when we were generating cache.

My idea was to detect what format is on the input as a source_image. Then use opm migrate -o yaml or json.
This would work for our use-case - bootstrapping - with empty target_image.

@JAVGan
Copy link
Contributor Author

JAVGan commented Apr 4, 2024

My idea was to detect what format is on the input as a source_image. Then use opm migrate -o yaml or json.
This would work for our use-case - bootstrapping - with empty target_image.

@lipoja I was thinking about this, but what would happen if we receive multiple operators (to add, for example) being one of them, say, JSON and the rest YAML? Or vice-versa? That's why I thought on streamlining everything, but sure, we can discuss this tomorrow.

@yashvardhannanavati
Copy link
Collaborator

My idea was to detect what format is on the input as a source_image. Then use opm migrate -o yaml or json.
This would work for our use-case - bootstrapping - with empty target_image.

@lipoja I was thinking about this, but what would happen if we receive multiple operators (to add, for example) being one of them, say, JSON and the rest YAML? Or vice-versa? That's why I thought on streamlining everything, but sure, we can discuss this tomorrow.

Yeah, I think agree with Jonathan's reasoning here since we are supporting adding multiple operators in a single fragment.

@lipoja
Copy link
Contributor

lipoja commented Apr 5, 2024

Yeah I know what you mean. I thought we could go with opm generating the output. The reason behind that is that opm is "consuming" those files so it would not generate anything that is faulty.
If we go this way I think we should validate the output -to be sure everything is correct and opm can load it.
opm validate might help us - in my opinion we should use this before every build.

@lipoja lipoja requested a review from xDaile April 8, 2024 13:36
@JAVGan JAVGan requested a review from lipoja April 8, 2024 19:33
@JAVGan
Copy link
Contributor Author

JAVGan commented Apr 10, 2024

Rebased

Copy link
Contributor

@xDaile xDaile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree to changes, however I IMO do not have enough expertise to approve this.

@JAVGan JAVGan requested a review from chandwanitulsi April 11, 2024 17:42
@JAVGan JAVGan force-pushed the yaml_index branch 2 times, most recently from 7c0b518 to 9cdf548 Compare April 16, 2024 17:29
chandwanitulsi
chandwanitulsi previously approved these changes Apr 16, 2024
JAVGan added 3 commits April 17, 2024 09:12
When an operator has catalog in yaml insted of json, some requests
(like add) fails on "duplicate package" as `merge_catalogs_dirs`
function generates FBC from hidden index.db created as json files
and then copy over its content to final location where are all yaml
files.

To prevent this issue this simple fix will walk through the config
directory and convert any YAML files into JSON to ensure that everything
uses the same format in the index image.

Refers to CLOUDDST-21432
This commit validates the JSON files from config dir after ensuring all
files are being present in this format.

Refers to CLOUDDST-21432
This commit changes the `merge_catalog_dirs` to call the
`enforce_json_config_dir` and `opm_validate` only once after merging all
files into the destination dir.

Refers to CLOUDDST-21432
@JAVGan
Copy link
Contributor Author

JAVGan commented Apr 17, 2024

@lipoja @chandwanitulsi @yashvardhannanavati I've rebased and fixed the conflicts, please give a final review so I can merge it

@JAVGan JAVGan merged commit e5b5d71 into master Apr 17, 2024
2 checks passed
@JAVGan JAVGan deleted the yaml_index branch April 17, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants