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

Enforce top level keys in autoinstall user data #1899

Conversation

Chris-Peterson444
Copy link
Contributor

Subiquity has, until now, silently ignored invalid top-level keys.
This has likely been a major source of confusion of autoinstall
capabilities.

The following autoinstall config:

version: 1
interative-sections:
    - identity
literally-anything: lmao

will now throw an AutoinstallValidationError.

@Chris-Peterson444
Copy link
Contributor Author

Chris-Peterson444 commented Jan 29, 2024

#1897 is a prerequisite for this PR. Once that is resolved, the only commit of interest here should be the latest (9375b1f)

@Chris-Peterson444
Copy link
Contributor Author

Additionally, should we consider this behavior change a breaking change? Since the interface hasn't changed, and I think the silent failures are more a bug than a feature (even though it was explicit), I think not. Opinions?

@Chris-Peterson444 Chris-Peterson444 marked this pull request as draft January 29, 2024 23:33
@Chris-Peterson444 Chris-Peterson444 force-pushed the enforce-top-level-keys-FR-5371 branch 3 times, most recently from 7170f85 to 38ab469 Compare February 2, 2024 01:17
@Chris-Peterson444 Chris-Peterson444 marked this pull request as ready for review February 2, 2024 01:18
@Chris-Peterson444 Chris-Peterson444 force-pushed the enforce-top-level-keys-FR-5371 branch 2 times, most recently from d8e0504 to 0b8b236 Compare February 2, 2024 01:25
Comment on lines +512 to +514
log.warning(
"Unrecognized keys may cause autoinstall to crash in future versions"
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently all this will do is print with WARNING to the log file. Do we see a need to use something like warning.warn to print to stderr instead? (Or stdout where the main server process is run)

Copy link
Collaborator

Choose a reason for hiding this comment

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

crash

I don't love this word, as the word crash will be seen to mean a defect.

stderr

No, that's not quite right. We need two cases to show the fault

  • interactive - we need to show the results in a dialog that indicates that the autoinstall is bad. The existing crash dialog is a good clue, but we won't reuse it directly because this is a non-defect case.
  • non-interactive - output needs to go to the same place as the rest of the UI, which may be on a serial console etc. Follow the existing context logging for that.

Also warn is the wrong mindset in that if we're supposed to be autoinstalling and we have stopped, then that is fatal to the program.

may cause

We have a distinct plan, it will cause, so let's set that text accordingly.

@Chris-Peterson444
Copy link
Contributor Author

Force pushed w/ updates to change from a run time error to a warning for now.

Subiquity currently ignores invalid top-level keys, but this has likely
been a major source of confusion of autoinstall capabilities.

In the future, the following autoinstall config will throw an
AutoinstallValidationError:

    version: 1
    interactive-sections:
        - identity
    literally-anything: lmao

This patch adds warnings to the logger and the machinery to
conditionally warn or error depending on the specified autoinstall
version.
):
self.message = f"Malformed autoinstall in {owner!r} section"
self.owner = owner
if not message:
Copy link
Collaborator

Choose a reason for hiding this comment

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

A standard across Subiquity is to prefer "is not None" over this case. I suspect that's a little better here.

message="top-level key 'version' is missing",
)

if version <= 1:
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we're adding v1/v2 behavior in this PR, then I want to see the autoinstall-reference updated at the same time.

Comment on lines +512 to +514
log.warning(
"Unrecognized keys may cause autoinstall to crash in future versions"
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

crash

I don't love this word, as the word crash will be seen to mean a defect.

stderr

No, that's not quite right. We need two cases to show the fault

  • interactive - we need to show the results in a dialog that indicates that the autoinstall is bad. The existing crash dialog is a good clue, but we won't reuse it directly because this is a non-defect case.
  • non-interactive - output needs to go to the same place as the rest of the UI, which may be on a serial console etc. Follow the existing context logging for that.

Also warn is the wrong mindset in that if we're supposed to be autoinstalling and we have stopped, then that is fatal to the program.

may cause

We have a distinct plan, it will cause, so let's set that text accordingly.

@Chris-Peterson444 Chris-Peterson444 marked this pull request as draft February 17, 2024 00:31
@Chris-Peterson444
Copy link
Contributor Author

Closing, superseded by #1947

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