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

Return a Config.Config in MachineInventory #35

Merged
merged 6 commits into from
Jul 13, 2022
Merged

Conversation

Itxaka
Copy link
Contributor

@Itxaka Itxaka commented Jul 12, 2022

Return a full config.Config with the Elemental key containing the usual
values used for the elemental-installer and the DATA key with the raw
values, so they can be interpreted by the elemental-installer to produce
the cloud-config to set in the machine.

Otherwise, we cant support cloud-config generic configurations like
write_files without updating the Machineinventory API every time we want
to add one, this way we allow setting an uncapped cloud-config in the
machineinventory and that will be passed down to the elemental-installer

Fixes #34

Would probably require first to get #32 properly set, so we can test this deeply with an authenticated request. Currently the tests only test the registrationURL unauthenticated, which doesn't produce the full output, only a limited output with just the elemental.registration fields filled and drops the rest. Once the auth part is fixed (cacerts I guess?) or I understand how to do an authenticated request, I would feel safe to merge this.

Signed-off-by: Itxaka igarcia@suse.com

Return a full config.Config with the Elemental key containing the usual
values used for the elemental-installer and the DATA key with the raw
values, so they can be interpreted by the elemental-installer to produce
the cloud-config to set in the machine.

Otherwise, we cant support cloud-config generic configurations like
write_files without updating the Machineinventory API every time we want
to add one, this way we allow setting an uncapped cloud-config in the
machineinventory and that will be passed down to the elemental-installer

Signed-off-by: Itxaka <igarcia@suse.com>
@Itxaka Itxaka requested a review from a team July 12, 2022 21:21
@kkaempf
Copy link
Contributor

kkaempf commented Jul 13, 2022

I'm missing documentation (resp. examples) of how a matching machineRegistration yaml would look like.
Otherwise 👍 from me.

Itxaka added 2 commits July 13, 2022 09:32
As we now use the annotations ont he data key in the Config.Config that
resulted in the installer outputting wrongly cloud-config files.

The fix is twofold, first when we marshal/unmarshal we add the omitempty
annotation so if Data is empty we dont use it and second, on the ToBytes
method we pop the data key from the values before returning the
cloud-config file, as we already merged the Data into the config, so the
key is useless

Signed-off-by: Itxaka <igarcia@suse.com>
It makes no sense to export to env vars the whole config as only the
elemental values are the ones needed by the installer/cli. Export only
that with the prefix ELEMENTAL_

Signed-off-by: Itxaka <igarcia@suse.com>
@codecov-commenter
Copy link

Codecov Report

Merging #35 (d335aed) into main (14d4d95) will decrease coverage by 0.18%.
The diff coverage is 29.41%.

@@            Coverage Diff             @@
##             main      #35      +/-   ##
==========================================
- Coverage   42.55%   42.36%   -0.19%     
==========================================
  Files           9        9              
  Lines         658      668      +10     
==========================================
+ Hits          280      283       +3     
- Misses        358      365       +7     
  Partials       20       20              
Impacted Files Coverage Δ
pkg/config/config.go 0.00% <0.00%> (ø)
pkg/server/register.go 13.87% <0.00%> (+0.15%) ⬆️
pkg/config/read.go 61.02% <100.00%> (+0.43%) ⬆️

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 14d4d95...d335aed. Read the comment docs.

Copy link
Contributor

@mjura mjura left a comment

Choose a reason for hiding this comment

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

LGTM

@Itxaka Itxaka requested review from davidcassany and a team July 13, 2022 08:24
Copy link
Contributor

@davidcassany davidcassany left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

I think later on, once the basic functionality is settled, we can spend some time thinking about how to better organize all the this data structures so irrelevant parts are not included.

elemental:
install:
device: /dev/vda
iso: https://something.example.com
Copy link
Contributor

@davidcassany davidcassany Jul 13, 2022

Choose a reason for hiding this comment

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

I understand the registration and system agent data of the config.Config.Elemental is irrelevant here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, registration data at least should be generated by the server (ca-cert, registration-url, token)

The system agent data I have no idea because Is till don't know what it does or even what format it should take...

},
Data: registration.Spec.Config.Data,
Copy link
Contributor

Choose a reason for hiding this comment

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

At this point it is unclear to me if the registration.Spec.Config.Elemental.Registration has some valid or valuable data at this point, I guess there should not be anything in there and if so it can be simply ignored right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I think this could probably be ignored as at this point in time, the machine running the elemental-installer has all this data for sure, I mean, otherwise it should not have reached this point.

I still think that leaving as is, its ok thought, that data may be used to be recorded somewhere? Like in the logs and so on, os it may be good for troubleshooting somehow? I dunno to be honest

@Itxaka Itxaka merged commit 278b9b2 into main Jul 13, 2022
@ldevulder ldevulder deleted the config_machineinventory branch August 3, 2022 12:17
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.

Move to config.Config instead of config.Install in MachineRegistration
5 participants