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

elemental-operator register: support config files in /oem subdirectories #76

Closed
wants to merge 1 commit into from

Conversation

fgiudici
Copy link
Member

We save the registration config.yaml file under /oem/registration dir.
The function we use to scan the /oem dir for files was not able to deal
with subdirectories:

FATA[0000] failed to read config /oem/registration/config.yaml: Config File "config.yaml" Not Found in "[/oem]"

@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2022

Codecov Report

Merging #76 (1c7625d) into main (9b9844b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #76   +/-   ##
=======================================
  Coverage   29.74%   29.74%           
=======================================
  Files           5        5           
  Lines         353      353           
=======================================
  Hits          105      105           
  Misses        244      244           
  Partials        4        4           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b9844b...1c7625d. Read the comment docs.

We save the registration config.yaml file under /oem/registration dir.
The function we use to scan the /oem dir for files was not able to deal
with subdirectories:

FATA[0000] failed to read config /oem/registration/config.yaml: Config File "config.yaml" Not Found in "[/oem]"

Signed-off-by: Francesco Giudici <[email protected]>
@davidcassany
Copy link
Contributor

I am not so sure about this change. I included this file in a subdirectory because in /oem we essentially store cloud-init yaml files that will be parsed by yip. Hence adding non cloud-init yaml files in there seams a bad approach to me, specially given the fact there is no schema validation in yip neither in elemental-operator. In fact I believe that having the user provided cloud-init files in /oem without a subfolder looks like a bad approach to me now (this is assumed in many places so we are not change it, at least for now).

All in all I don't think it is a good idea to keep the registration yaml file together with the cloud init files as yip will attempt to load the registration yaml file as a cloud-init file and elemental-operator will load (and merge) all yaml files it finds and attempt to match them into a config struct.

All that to say I believe that the default /oem folder for the elemental-operator is just wrong IMHO and that probably it should be /oem/registration or whatever we decide.

In such a case the patch of this PR won't be needed. What you think?

@fgiudici
Copy link
Member Author

I am not so sure about this change. I included this file in a subdirectory because in /oem we essentially store cloud-init yaml files that will be parsed by yip. Hence adding non cloud-init yaml files in there seams a bad approach to me, specially given the fact there is no schema validation in yip neither in elemental-operator. In fact I believe that having the user provided cloud-init files in /oem without a subfolder looks like a bad approach to me now (this is assumed in many places so we are not change it, at least for now).

All in all I don't think it is a good idea to keep the registration yaml file together with the cloud init files as yip will attempt to load the registration yaml file as a cloud-init file and elemental-operator will load (and merge) all yaml files it finds and attempt to match them into a config struct.

All that to say I believe that the default /oem folder for the elemental-operator is just wrong IMHO and that probably it should be /oem/registration or whatever we decide.

In such a case the patch of this PR won't be needed. What you think?

Yeah, I completely agree with you, we should keep the registration information somewhere else than could-init files (that the elemental-operator register command processes right now!!).
The patch here is a dumb one: it just allows elemental-operator register to parse the reg file we save in /oem/registration/config.yaml by processing subfolders under oem, so that we are able to run the elemental-operator register command later and it will be able to get the URL.
This deserve a better rework, closing this PR for now

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.

3 participants