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

updata add-update: don't create manifest, require 'init' first #991

Merged

Conversation

tjkirch
Copy link
Contributor

@tjkirch tjkirch commented Jul 13, 2020

Automatically creating a manifest during add-update can be problematic if you
thought the manifest path did exist. You may create an empty repo instead of
adding to one as intended.

This change requires "init" to be run first, so you're showing your intention
to create a new repo.


This happened to us during the 0.4.1 release when making a test repo that we expected to include older versions as well.

Testing done:

Before, it runs regardless of whether the manifest ("not-a-file") exists:

$ cargo run --bin updata -- add-update not-a-file -a x86_64 -b README.md -h README.md -r README.md -v 0.0.1 -m 0.0.1 -f variant

It creates an empty manifest, then inserts the update, when I intended to add to an existing repo:

$ cat not-a-file
{
  "updates": [
     ...

After, it fails in the expected way when the file doesn't exist. (Works as expected after initing the file.)

$ cargo run --bin updata -- add-update not-a-file -a x86_64 -b README.md -h README.md -r README.md -v 0.0.1 -m 0.0.1 -f variant
23:05:52 [ERROR] Failed to read manifest file 'not-a-file' - do you need to `updata init`? (No such file or directory (os error 2))

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

Automatically creating a manifest during `add-update` can be problematic if you
thought the manifest path did exist.  You may create an empty repo instead of
adding to one as intended.

This change requires "init" to be run first, so you're showing your intention
to create a new repo.
Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

🥇

@tjkirch tjkirch merged commit 94b5329 into bottlerocket-os:develop Jul 14, 2020
@tjkirch tjkirch deleted the updata-fail-missing-manifest branch July 14, 2020 19:38
tjkirch added a commit to tjkirch/bottlerocket that referenced this pull request Jul 20, 2020
Missed this when adding init requirement in
bottlerocket-os#991
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.

None yet

4 participants