Skip to content

Fix agama config show before a product is selected#2066

Merged
mvidner merged 4 commits intomasterfrom
storage-config-null
Feb 25, 2025
Merged

Fix agama config show before a product is selected#2066
mvidner merged 4 commits intomasterfrom
storage-config-null

Conversation

@mvidner
Copy link
Copy Markdown
Contributor

@mvidner mvidner commented Feb 24, 2025

Problem

Solution

Return an empty JSON object (Ruby Hash) instead of null/nil

Adapt the Rust client to deal with the null and omit the section.

Testing

  • Tested manually:
    • ./testing-using-container.sh and in its shell, agama config show
    • Web UI still works normally
  • Added a unit test

Debugging

git bisect saved me

Screenshots

No

tell the user, not just the logs, when:
```
abusctl call org.opensuse.Agama.Storage1 /org/opensuse/Agama/Storage1 org.opensuse.Agama.Storage1 GetConfig
s "null"
```
@mvidner
Copy link
Copy Markdown
Contributor Author

mvidner commented Feb 24, 2025

Not related to the issue/fix, but here's the story of finding the bug cause, just because I am happy that it worked out well 😃

I tried to test the v11 tag and luckily the bug was not there. But that was five weeks ago! I must automate! Let's bisect.

1899 was a nearby revision where my testing container worked at all.

(master|BISECTING) martin@ohmu:~/svn/agama> git bisect good pr/1899
Bisecting: 425 revisions left to test after this (roughly 9 steps)

425 revisions, but the computer helped me persist, yay!

Here's the output of git bisect log after the successful hunt.
My testing command was abusctl call org.opensuse.Agama.Storage1 /org/opensuse/Agama/Storage1 org.opensuse.Agama.Storage1 GetConfig, run in the ./testing-using-container.sh shell,
returning {} for good, null for bad and when I thought I was in the middle of a breaking change I dared to skip.
In a perfect world, one would be able to run a test command and have it succeed or fail and let git bisect run work fully automatically,
but in practice the testing fails in ways not related to the bug being hunted, so you either craft an idempotent testing harness or repeat some workarounds by hand, and I did the latter.

git bisect start
# status: waiting for both good and bad commits
# bad: [09433a4b3ce4f242a245bb19fe95bc7744ce7c19] Temporarily hide the abort installation option (#2056)
git bisect bad 09433a4b3ce4f242a245bb19fe95bc7744ce7c19
# status: waiting for good commit(s), bad commit known
# good: [9a4a8ecd27e5a5dd17b57f2cee7082d634e65f6c] service: changelog
git bisect good 9a4a8ecd27e5a5dd17b57f2cee7082d634e65f6c
# bad: [6bc6a712c2803a0c7077cf5dcf9a07c126ed8daa] fix(web): another relocation of storage useConfigModel
git bisect bad 6bc6a712c2803a0c7077cf5dcf9a07c126ed8daa
# bad: [fb4e04ce05cc65d7f4a26341951a9e74a8e0e6ef] feat(web): Migration to PatternFly v6, round 2 (#1921)
git bisect bad fb4e04ce05cc65d7f4a26341951a9e74a8e0e6ef
# bad: [fb4e04ce05cc65d7f4a26341951a9e74a8e0e6ef] feat(web): Migration to PatternFly v6, round 2 (#1921)
git bisect bad fb4e04ce05cc65d7f4a26341951a9e74a8e0e6ef
# skip: [cbf0d39a49b9e1912e73eea723e38cad73511c58] feat(web): allow changing boot options
git bisect skip cbf0d39a49b9e1912e73eea723e38cad73511c58
# skip: [a108175c823855a5883cf5c4f076a124de9e4ab0] feat(storage): use proper space policy
git bisect skip a108175c823855a5883cf5c4f076a124de9e4ab0
# skip: [fe814eeb1f32ee5e6bfd52fb59e86ae2b5654858] feat(web): update config-model types
git bisect skip fe814eeb1f32ee5e6bfd52fb59e86ae2b5654858
# good: [89db30e35319226aaf8b7b70d13b5b6af78216e9] web: Improvements from code review
git bisect good 89db30e35319226aaf8b7b70d13b5b6af78216e9
# good: [89db30e35319226aaf8b7b70d13b5b6af78216e9] web: Improvements from code review
git bisect good 89db30e35319226aaf8b7b70d13b5b6af78216e9
# bad: [65439820e6c41f543d715d4fcb4e6c3d02fbb4a2] Merge branch 'master' into storage-config-ui-merge-master
git bisect bad 65439820e6c41f543d715d4fcb4e6c3d02fbb4a2
# bad: [65439820e6c41f543d715d4fcb4e6c3d02fbb4a2] Merge branch 'master' into storage-config-ui-merge-master
git bisect bad 65439820e6c41f543d715d4fcb4e6c3d02fbb4a2
# bad: [4c0e887bfc12a93423856167a4222987e76860c7] Fix some unit tests
git bisect bad 4c0e887bfc12a93423856167a4222987e76860c7
# bad: [7d2d581c3a25d9d7a67e6aa1859130f3647cfbe9] chore: merge master (#1786)
git bisect bad 7d2d581c3a25d9d7a67e6aa1859130f3647cfbe9
# good: [e39588defabb7381780ad75a6873f44ccfe9dcc6] feat(web): add hook to get config model
git bisect good e39588defabb7381780ad75a6873f44ccfe9dcc6
# bad: [90afa5ffea355c80e14f402430a013c7859bb5eb] chore(web): drop storage config model
git bisect bad 90afa5ffea355c80e14f402430a013c7859bb5eb
# bad: [061aa44a07a2cae31da5c4196e22481dbddacdf5] chore(storage): drop solved_config HTTP endpoint
git bisect bad 061aa44a07a2cae31da5c4196e22481dbddacdf5
# bad: [b9e82942222945f5c2e9f36b189529c83f5986eb] chore(storage): drop GetSolvedConfig D-Bus method
git bisect bad b9e82942222945f5c2e9f36b189529c83f5986eb
# first bad commit: [b9e82942222945f5c2e9f36b189529c83f5986eb] chore(storage): drop GetSolvedConfig D-Bus method

Copy link
Copy Markdown
Contributor Author

@mvidner mvidner left a comment

Choose a reason for hiding this comment

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

Funny how I cannot Request changes on my own PR :)

legacyAutoyastStorage: JSON.parse(strategy.settings.to_json, symbolize_names: true)
}
else
{}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

On a call we have clarified that the null from D-Bus is desired, the Rust client should be fixed to handle it instead

@mvidner mvidner force-pushed the storage-config-null branch from 3c9438c to 027465f Compare February 25, 2025 13:33
let store = storage_store(url);
let settings = store.load().await?;
let opt_settings = store.load().await?;
assert!(opt_settings.is_some());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this condition is not met, the assert! call will crash the program. Is that what we want? However, it is just the CLI, so I guess we can live with that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ENOCOFFEE? This is the test module not the CLI :)

Copy link
Copy Markdown
Contributor

@imobachgs imobachgs Feb 25, 2025

Choose a reason for hiding this comment

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

And if crashing is expected anyway, I would say that using expect is more Rust-like.

let settings = op_settings.expect("...")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well, let's grab another coffee :-)

Copy link
Copy Markdown
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

LGTM

let store = storage_store(url);
let settings = store.load().await?;
let opt_settings = store.load().await?;
assert!(opt_settings.is_some());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well, let's grab another coffee :-)

@mvidner mvidner merged commit 073e016 into master Feb 25, 2025
6 checks passed
@mvidner mvidner deleted the storage-config-null branch February 25, 2025 15:38
@imobachgs imobachgs mentioned this pull request Feb 26, 2025
imobachgs added a commit that referenced this pull request Feb 26, 2025
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.

2 participants