Fix image size calculation #499
Conversation
Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>
Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>
Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>
jimmykarily
force-pushed
the
fix-image-size-calculation-tests
branch
from
40ea4d2
to
69e7e6a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #499 +/- ##
==========================================
- Coverage 52.07% 52.00% -0.07%
==========================================
Files 52 52
Lines 4941 4951 +10
==========================================
+ Hits 2573 2575 +2
- Misses 2083 2087 +4
- Partials 285 289 +4 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>
jimmykarily
changed the title
|
With kairos-agent from this branch, I still see this text: on the ugprade logs. Either the text is wrong or it didn't work as expected. |
there are 3 ways: - cli arg: --recovery - cli arg: --boot-entry - config setting: upgrade.recovery: true We only checked the config setting Signed-off-by: Dimitris Karakasilis <dimitris@karakasilis.me>
| @@ -238,12 +238,14 @@ func upgradeUki(source string, dirs []string, upgradeEntry string, strictValidat | |||
| // ExtraConfigUpgrade is the struct that holds the upgrade options that come from flags and events | |||
| type ExtraConfigUpgrade struct { | |||
There was a problem hiding this comment.
We take the cli args and we make a new type of config (this one). Then we serialize this thing to a json (or YAML?) string and we merge it with the rest of the config. The keys of the resulting string didn't match those of the Config we are trying to build thus the setting only made it to the raw Config (which we store as a separate key). That's why the original PR (see the first commit in this one) had to use cfg.Query to get to this key.
With the changes here, the actual fields of the Config get populated correctly.
| entry := "" | ||
| _, ok := cfg.Config["upgrade"] | ||
| if ok { | ||
| // check value from --recovery and --boot-entry | ||
| entry, _ = cfg.Config["upgrade"].(collector.Config)["entry"].(string) |
There was a problem hiding this comment.
We were skipping this but no tests were broken. This means we need to add a test to make sure we don't break it again.
There was a problem hiding this comment.
It seems that this wasn't needed because of the unmarshallFullSpec below: https://github.com/kairos-io/kairos-agent/pull/499/files#diff-53b724197bc14c3828ce6725322b892cb788860b0526184b63d7d114edb60941R370
generateUpgradeConfForCLIArgs has added the field upgrade.entry (see ExtraConfigUpgrade) which is marshalled into a yaml and passed to the config collector (as a string).
Let's check if this is still working as intended.
| @@ -146,8 +146,8 @@ func generateUpgradeConfForCLIArgs(source, upgradeEntry string) (string, error) | |||
| // have access to that yet, we just set both uri values which shouldn't matter | |||
| // anyway, the right one will be used later in the process. | |||
| if source != "" { | |||
| upgradeConfig.Upgrade.RecoverySystem.URI = source | |||
| upgradeConfig.Upgrade.System.URI = source | |||
| upgradeConfig.Install.RecoverySystem.URI = source | |||
There was a problem hiding this comment.
this feels wrong. This has worked with cli flags since forever, so it doesnt seem like it would be the source of the issue. The end config/spec has the proper sources so it seems wrong to me that this would be broken here?
Are we sure this doesnt break anything? IIRC our test use the cli flags to upgrade by setting a source, so this seems to be working as expected or our tests are fully broken.
I dont have anything else to proof this other than a feeling that if source cli flags work, this migth be working but I could be wrong...
|
We discussed this in the planning meeting and we agreed this part of the code needs some refactoring but for now we'll go with this fix instead: https://github.com/kairos-io/kairos-agent/pull/508/files I will try to move the new test from this PR to the #508 one to have it as an additional check. |
|
I created this PR for the test and the additional fix: #509 |
Supersedes https://github.com/kairos-io/kairos-agent/pull/467/files