Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

OTA-2418: Remove example.com URL from automated garage-sign usage #1169

Merged
merged 1 commit into from
Apr 4, 2019

Conversation

mike-sul
Copy link
Collaborator

@mike-sul mike-sul commented Apr 4, 2019

Signed-off-by: Mike Sul ext-mykhaylo.sul@here.com

Verification steps:
make check & make qa

Bitbaking and running 'happy path' scenarios on qemu, see this PR advancedtelematic/meta-updater#510 for more details.

Copy link
Collaborator

@pattivacek pattivacek left a comment

Choose a reason for hiding this comment

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

Thanks! So the real test here will be to bitbake some images without the URL set with your new meta-updater branch, and then use this fixed version of garage-deploy to copy them to another account, and then verify that a device can update successfully with the copied objects in the second account. Does that make sense? I seem to recall that being the thing that broke way back when, and hopefully it'll work this time.

if (custom_uri != "https://example.com/") {
uri_ = std::move(custom_uri);
}
uri_ = std::move(custom_uri);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately we probably have to keep this special rule in place for backwards compatibility. I wish we could remove it but it will break things if we do.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All tests passed, so I am wondering what are the things it will break ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are thousands of objects in production/SIT whose Target metadata lists a URL as "example.com". If someone were to use a version of libaktualizr that has my recent fix to actually use that URL field but does not know to special-case and ignore that specific URL, they won't be able to update to those targets any more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks for explanation.

target_json["custom"]["uri"] = "https://example.com/";
target_json["length"] = 1;
Uptane::Target target("fake_file", target_json);

Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll have to keep this test case, too.

@codecov-io
Copy link

codecov-io commented Apr 4, 2019

Codecov Report

Merging #1169 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1169   +/-   ##
=======================================
  Coverage   77.88%   77.88%           
=======================================
  Files         170      170           
  Lines       10052    10052           
=======================================
  Hits         7829     7829           
  Misses       2223     2223
Impacted Files Coverage Δ
src/sota_tools/deploy.cc 51.38% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c020de...16bfc5e. Read the comment docs.

Signed-off-by: Mike Sul <ext-mykhaylo.sul@here.com>
@mike-sul mike-sul force-pushed the feat/OTA-2418/remove-example.com branch from f0ed5e3 to 16bfc5e Compare April 4, 2019 09:24
@mike-sul
Copy link
Collaborator Author

mike-sul commented Apr 4, 2019

Updated according to the comments.

@pattivacek pattivacek merged commit 4655bf8 into master Apr 4, 2019
@pattivacek pattivacek deleted the feat/OTA-2418/remove-example.com branch April 4, 2019 13:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants