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

The manifest_version should default to 1 if missing. #214

Merged
merged 4 commits into from
Mar 16, 2021

Conversation

Integralist
Copy link
Collaborator

@Integralist Integralist commented Mar 16, 2021

Problem: Originally if the manifest_version field was missing from the fastly.toml manifest file, then we would return an error.
Solution: We should instead set a default value of 1.

Notes: to issue an appropriate “warning” message, it required the manifest File object to have access to a consistent output stream. This meant everywhere we call c.manifest.File.Read(manifest.Filename) (which is in every command) I’ve had to precede it with a call to c.manifest.File.SetOutput(c.Globals.Output).

@Integralist Integralist added the bug Something isn't working label Mar 16, 2021
@Integralist Integralist requested a review from kailan March 16, 2021 10:22
Copy link
Member

@kailan kailan left a comment

Choose a reason for hiding this comment

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

Fantastic! Glad this got caught.

Copy link
Member

@phamann phamann left a comment

Choose a reason for hiding this comment

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

Minor nit:

pkg/compute/manifest/manifest.go Show resolved Hide resolved
Copy link
Member

@phamann phamann left a comment

Choose a reason for hiding this comment

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

:shipit:

@Integralist Integralist force-pushed the integralist/20210316_manifest_default_1 branch from a58de47 to 31f3849 Compare March 16, 2021 10:51
@Integralist
Copy link
Collaborator Author

Kailan, Patrick and myself have discussed the implementation internally and settled on a solution (which is now implemented).

@Integralist Integralist merged commit c8f0aa8 into master Mar 16, 2021
@Integralist Integralist deleted the integralist/20210316_manifest_default_1 branch March 16, 2021 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants