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.

Sign up for GitHub

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

Jump to bottom

feat: Integration with Cumulocity CA #3259

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

feat: Integration with Cumulocity CA #3259

wants to merge 12 commits into from

Conversation

didier-wenzek
Copy link
Contributor

@didier-wenzek didier-wenzek commented Nov 21, 2024 •
edited
Loading

Proposed changes

  • Impl tedge cert download c8y
  • Impl tedge cert renew c8y
  • Enhance tedge config with setting for certificate validity period.
  • Enhance tedge cert show to display number of days before expiration.
  • Make sure the current certificate is not erased on renewal

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

#3248

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

@didier-wenzek didier-wenzek added feature Change request theme:c8y Theme: Cumulocity related topics theme:certificates labels Nov 21, 2024
@codecov Codecov
Copy link

codecov bot commented Nov 21, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 5.71429% with 231 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...tes/core/tedge/src/cli/certificate/c8y/download.rs 0.00% 81 Missing ⚠️
crates/core/tedge/src/cli/certificate/c8y/renew.rs 0.00% 52 Missing ⚠️
crates/core/tedge/src/cli/certificate/cli.rs 0.00% 41 Missing ⚠️
crates/core/tedge/src/cli/certificate/c8y/mod.rs 0.00% 29 Missing ⚠️
.../tedge_config/src/tedge_config_cli/tedge_config.rs 48.00% 10 Missing and 3 partials ⚠️
crates/core/tedge/src/cli/certificate/mod.rs 0.00% 9 Missing ⚠️
crates/core/tedge/src/cli/certificate/create.rs 28.57% 3 Missing and 2 partials ⚠️
crates/core/c8y_api/src/json_c8y_deserializer.rs 0.00% 1 Missing ⚠️
Additional details and impacted files

📢 Thoughts on this report? Let us know!

didier-wenzek
didier-wenzek commented Nov 21, 2024
View reviewed changes
crates/core/tedge/src/cli/certificate/download.rs Outdated Show resolved Hide resolved
crates/core/tedge/src/cli/certificate/download.rs Outdated Show resolved Hide resolved
crates/core/tedge/src/cli/certificate/download.rs Outdated
)
}
}
warning!("Will retry in 5 seconds");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One needs to loop only when it makes senses to retry: i.e. the c8y endpoint has been reached but the device has not been registered yet.

Question: does Cumulocity return a specific HTTP status when the device is not registered?

crates/core/tedge/src/cli/certificate/download.rs Outdated Show resolved Hide resolved
crates/core/tedge/src/cli/certificate/download.rs Outdated Show resolved Hide resolved
@didier-wenzek didier-wenzek force-pushed the feat/integration-with-cumulocity-ca branch from 844dc34 to 284c8ee Compare November 22, 2024 10:16
@didier-wenzek didier-wenzek force-pushed the feat/integration-with-cumulocity-ca branch from 284c8ee to fba8ed6 Compare November 22, 2024 16:00
@didier-wenzek didier-wenzek force-pushed the feat/integration-with-cumulocity-ca branch from 5f284b5 to 5335af8 Compare November 28, 2024 13:46
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 28, 2024 •
edited
Loading

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
529 0 2 529 100 1h33m25.693521s

@didier-wenzek didier-wenzek force-pushed the feat/integration-with-cumulocity-ca branch from 8ee9b20 to abfa57e Compare December 4, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@albinsuresh albinsuresh Awaiting requested review from albinsuresh albinsuresh will be requested when the pull request is marked ready for review albinsuresh is a code owner

@jarhodes314 jarhodes314 Awaiting requested review from jarhodes314 jarhodes314 will be requested when the pull request is marked ready for review jarhodes314 is a code owner

@rina23q rina23q Awaiting requested review from rina23q rina23q will be requested when the pull request is marked ready for review rina23q is a code owner

@reubenmiller reubenmiller Awaiting requested review from reubenmiller reubenmiller will be requested when the pull request is marked ready for review reubenmiller is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
feature Change request theme:c8y Theme: Cumulocity related topics theme:certificates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@didier-wenzek @reubenmiller