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

stages/files: add previousReport to the result report if Ignition is run more than once #1254

Merged
merged 1 commit into from
Aug 5, 2021

Conversation

sohankunkerkar
Copy link
Member

This change notices if a report already exists and accordingly nest the report in previousReport.
related to #1214

Based on this change, the fedora-coreos-config code would check if previousReport exists and could throw a fat warning saying Ignition was previously run at xx. Unexpected behavior may occur. Ignition is not designed to run more than once per system.

Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

General approach LGTM. 👍

internal/exec/stages/files/filesystemEntries.go Outdated Show resolved Hide resolved
internal/exec/stages/files/filesystemEntries.go Outdated Show resolved Hide resolved
return fmt.Errorf("reading previous report: %w", err)
}
if err == nil {
err = json.Unmarshal(prevData, &prevReport)
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, let's log a warning in this case too, similar to the one in coreos/fedora-coreos-config#1148.

Copy link
Member

@jlebon jlebon Aug 5, 2021

Choose a reason for hiding this comment

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

Shouldn't we do this only if we're not on a live system? Otherwise, this will be forever stacking previousReports on each live boot.

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't we do this only if we're not on a live system? Otherwise, this will be forever stacking previousReports on each live boot.

Yeah, that's a good point!

Copy link
Contributor

Choose a reason for hiding this comment

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

Each of the stacked Ignition runs affect the live system, so my thinking was that recording the run history was useful. But I'm okay dropping it if we want.

Copy link
Member

Choose a reason for hiding this comment

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

I'm OK adding onto it I guess, though continually nesting when you know it could keep going forever by design feels weird. Would be more natural if it were a list instead, i.e. previousReports[].

@sohankunkerkar sohankunkerkar changed the title [WIP] stages/files: add previousReport to the result report if Ignition is run more than once stages/files: add previousReport to the result report if Ignition is run more than once Aug 3, 2021
@sohankunkerkar sohankunkerkar force-pushed the ignition-prevReport branch 2 times, most recently from 2ea3db2 to 23e44da Compare August 4, 2021 21:18
internal/exec/stages/files/filesystemEntries.go Outdated Show resolved Hide resolved
internal/exec/stages/files/filesystemEntries.go Outdated Show resolved Hide resolved
internal/exec/stages/files/filesystemEntries.go Outdated Show resolved Hide resolved
internal/exec/stages/files/filesystemEntries.go Outdated Show resolved Hide resolved
internal/exec/stages/files/filesystemEntries.go Outdated Show resolved Hide resolved
internal/exec/stages/files/filesystemEntries.go Outdated Show resolved Hide resolved
internal/exec/stages/files/filesystemEntries.go Outdated Show resolved Hide resolved
This change notices if a report already exists and accordingly
nest the report in previousReport.
related to coreos#1214
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