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

using garage-check automatically at the end of garage-deploy #1244

Merged
merged 1 commit into from
Jul 5, 2019

Conversation

Zee314159
Copy link
Contributor

Draft code.

@Zee314159 Zee314159 force-pushed the garage-deploy-use-garage-check branch from 6375dcd to 6298745 Compare June 26, 2019 12:00
@codecov-io
Copy link

codecov-io commented Jun 26, 2019

Codecov Report

Merging #1244 into master will increase coverage by 0.21%.
The diff coverage is 66.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1244      +/-   ##
==========================================
+ Coverage   79.46%   79.68%   +0.21%     
==========================================
  Files         169      170       +1     
  Lines       10136    10144       +8     
==========================================
+ Hits         8055     8083      +28     
+ Misses       2081     2061      -20
Impacted Files Coverage Δ
src/sota_tools/garage_deploy.cc 89.47% <100%> (-0.94%) ⬇️
src/sota_tools/garage_check.cc 59.01% <100%> (+1.87%) ⬆️
src/sota_tools/check.cc 64.28% <64.28%> (ø)
src/libaktualizr/primary/sotauptaneclient.cc 93.43% <0%> (+0.76%) ⬆️
src/libaktualizr/uptane/imagesrepository.cc 93.2% <0%> (+1.23%) ⬆️
src/libaktualizr/storage/sqlstorage.cc 77.22% <0%> (+1.28%) ⬆️

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 3a13ffa...17fc673. Read the comment docs.

@@ -122,6 +122,14 @@ int main(int argc, char **argv) {
LOG_FATAL << e.what();
return EXIT_FAILURE;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to have a check for dry run, like in the rest of the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that makes sense! Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't tested the code yet, just pushed it here to get your advices. Am I on the right track?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's going in the right direction. I would just move CheckRefValid in a new source file.

@@ -122,6 +122,14 @@ int main(int argc, char **argv) {
LOG_FATAL << e.what();
return EXIT_FAILURE;
}

std::string cmd("garage check");
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to have a typo, garage-check is the correct tool name.

But also, it would be best to not launch another process and just call some C++ code as garage-check's code lives in this repo. This way, garage-check wouldn't need to be in the $PATH.

It would probably require a small refactor to move garage-check's logic in a small library that would be used by garage-check and garage-deploy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems a bit complicated than I thought, but this looks like the right way to do it. Thank you!

@Zee314159 Zee314159 force-pushed the garage-deploy-use-garage-check branch 4 times, most recently from 615c252 to 68cf26b Compare July 2, 2019 04:40
@Zee314159
Copy link
Contributor Author

I tested the garage-deploy tool, looks good. The log showed the process succeed. I checked my other test account, the image I deployed was present on the server. Is there anything else I should test or fix?

@@ -77,18 +81,18 @@ bool OfflineSignRepo(const ServerCredentials &push_credentials, const std::strin
boost::filesystem::remove_all(local_repo);
}

std::string init_cmd("garage-sign init --repo aktualizr --credentials ");
std::string init_cmd("./garage-sign/bin/garage-sign init --repo aktualizr --credentials ");
Copy link
Contributor

Choose a reason for hiding this comment

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

I would switch back to init_cmd("garage-sign init --repo aktualizr --credentials ");, this would make problems in the actual released code.

@@ -43,4 +43,10 @@ bool OfflineSignRepo(const ServerCredentials& push_credentials, const std::strin
bool PushRootRef(const ServerCredentials& push_credentials, const OSTreeRef& ref, const std::string& cacerts,
RunMode mode);

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok that works but it's maybe a bit confusing to have this function as part of deploy.cc. Maybe just add it to a new translation unit check.cc and check.h.

@Zee314159 Zee314159 force-pushed the garage-deploy-use-garage-check branch 5 times, most recently from da060ca to fc4988f Compare July 3, 2019 06:50
}

int CheckRefValid(const TreehubServer &treehub, const std::string &ref, RunMode mode, int max_curl_requests) {
// vim: set tabstop=2 shiftwidth=2 expandtab:
Copy link
Contributor

Choose a reason for hiding this comment

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

This vim comment shouldn't probably be here.

@lbonn
Copy link
Contributor

lbonn commented Jul 4, 2019

Only a very small nitpick and it should be good.

@Zee314159 Zee314159 force-pushed the garage-deploy-use-garage-check branch 3 times, most recently from f56b1c9 to 6581cff Compare July 5, 2019 08:49
Moved garage-check's logic in a small library

Added check.cc and check.h

Deleted the vim comment

Signed-off-by: Zee314159 <252806294@qq.com>
@Zee314159 Zee314159 force-pushed the garage-deploy-use-garage-check branch from 6581cff to 17fc673 Compare July 5, 2019 08:51
@lbonn lbonn merged commit a4d36a9 into master Jul 5, 2019
@pattivacek pattivacek deleted the garage-deploy-use-garage-check branch July 10, 2019 14:52
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.

None yet

3 participants