Skip to content

fix: tedge cert create-csr should use cloud profile's CN#3316

Merged
rina23q merged 2 commits intothin-edge:mainfrom
rina23q:fix/3315/cert-create-csr-should-use-cloud-profile-cn
Jan 10, 2025
Merged

fix: tedge cert create-csr should use cloud profile's CN#3316
rina23q merged 2 commits intothin-edge:mainfrom
rina23q:fix/3315/cert-create-csr-should-use-cloud-profile-cn

Conversation

@rina23q
Copy link
Copy Markdown
Member

@rina23q rina23q commented Jan 6, 2025

Proposed changes

tedge cert create-csr <cloud> --profile <profile> should use the CN of the corresponding device certificate/private key if --device-id is not provided.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

#3315

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 6, 2025

Codecov Report

Attention: Patch coverage is 0% with 16 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/core/tedge/src/cli/certificate/cli.rs 0.00% 16 Missing ⚠️
Additional details and impacted files

📢 Thoughts on this report? Let us know!

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jan 6, 2025

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
552 0 2 552 100 1h29m9.064176999s

@rina23q rina23q temporarily deployed to Test Pull Request January 6, 2025 16:37 — with GitHub Actions Inactive

// Use the current device id if no id is provided
let id = id.unwrap_or(get_device_id_from_cloud(&config, &cloud)?);
let id = if let Some(id) = id {
Copy link
Copy Markdown
Member Author

@rina23q rina23q Jan 6, 2025

Choose a reason for hiding this comment

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

At first, I wrote this line as below.

let id = id.unwrap_or(get_device_id_from_cloud(&config, &cloud)?);

However, it causes a problem when certificate doesn't exist but the id has Some() sinceunwrap_or() evaluates the default always regardless of id is Some or None.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Indeed, and unwrap_or_else, makes it so you can't use ? from within the closure, which is a bit of a pain.

@rina23q rina23q force-pushed the fix/3315/cert-create-csr-should-use-cloud-profile-cn branch from f9bab9b to 5f6a6f2 Compare January 6, 2025 17:26
@rina23q rina23q temporarily deployed to Test Pull Request January 6, 2025 17:27 — with GitHub Actions Inactive

// Use the current device id if no id is provided
let id = id.unwrap_or(get_device_id_from_cloud(&config, &cloud)?);
let id = if let Some(id) = id {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Indeed, and unwrap_or_else, makes it so you can't use ? from within the closure, which is a bit of a pain.

Copy link
Copy Markdown
Contributor

@albinsuresh albinsuresh left a comment

Choose a reason for hiding this comment

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

LGTM

@rina23q rina23q force-pushed the fix/3315/cert-create-csr-should-use-cloud-profile-cn branch from 5f6a6f2 to ed05f1d Compare January 9, 2025 15:57
@rina23q rina23q added bug Something isn't working theme:cli Theme: cli related topics labels Jan 9, 2025
Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>
…rofile

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>
@rina23q rina23q force-pushed the fix/3315/cert-create-csr-should-use-cloud-profile-cn branch from ed05f1d to a4d99a4 Compare January 10, 2025 10:34
@rina23q rina23q temporarily deployed to Test Pull Request January 10, 2025 10:34 — with GitHub Actions Inactive
@rina23q rina23q added this pull request to the merge queue Jan 10, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 10, 2025
@rina23q rina23q added this pull request to the merge queue Jan 10, 2025
Merged via the queue into thin-edge:main with commit 87a9681 Jan 10, 2025
@rina23q rina23q mentioned this pull request Jan 10, 2025
15 tasks
@reubenmiller reubenmiller added this to the 1.4.2 milestone Jan 14, 2025
@rina23q rina23q deleted the fix/3315/cert-create-csr-should-use-cloud-profile-cn branch August 8, 2025 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working theme:cli Theme: cli related topics

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants