-
Notifications
You must be signed in to change notification settings - Fork 60
Conversation
It's already done in the authenticate function. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>
src/sota_tools/garage_deploy.cc
Outdated
} | ||
|
||
TreehubServer push_server; | ||
if (authenticate(cacerts, push_credentials, push_server) != EXIT_SUCCESS) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't push_server authenticate be done before 'UploadToTreehub'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I've accidentally made it authenticate twice now. I'll try to refactor this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
src/sota_tools/CMakeLists.txt
Outdated
@@ -317,7 +317,7 @@ if (BUILD_SOTA_TOOLS) | |||
add_test(NAME garage-deploy-online-signing | |||
COMMAND ${PROJECT_SOURCE_DIR}/tests/sota_tools/test-garage-deploy-online-signing $<TARGET_FILE:garage-deploy> | |||
WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}) | |||
set_tests_properties(garage-deploy-online-signing PROPERTIES PASS_REGULAR_EXPRESSION "Online signing with garage-deploy is currently unsupported") | |||
set_tests_properties(garage-deploy-online-signing PROPERTIES PASS_REGULAR_EXPRESSION "Online signing with garage-deploy is not supported") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this means we are not planning to support online signing anytime soon or at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Online signing" here has kind of a funny meaning. We just shouldn't use that phrase here, because it's confusing with how we use the term elsewhere. I'll change it.
To explain: normally, your credentials file has a key for the Targets role of the Image repository. This is true even if you haven't rotated that key (e.g. what we normally call "offline signing"). If that key isn't there, it's a problem.
Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>
cf11573
to
e4bd536
Compare
Codecov Report
@@ Coverage Diff @@
## master #1347 +/- ##
==========================================
- Coverage 80.11% 80.05% -0.06%
==========================================
Files 178 178
Lines 10559 10558 -1
==========================================
- Hits 8459 8452 -7
- Misses 2100 2106 +6
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Garage-deploy tests passed:
Deploy unique ostree image - OK
Deploy existing ostree image - OK
Deploy binary image doesn't succeed - OK
Prevents authenticating twice in garage-deploy. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>
e4bd536
to
d41c09a
Compare
I accidentally broke the docs build but just now pushed a fix (no change to the actual code) which auto-dismissed your review. |
This was a minor logic problem (with detecting when we can't sign the targets) caught by OTF, but while looking closer, I realized there was a bigger problem: we were running the garage-check logic on the fetch repo instead of the push repo. I fixed that and addressed the online signing logic flow as well.