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

el2g: add documentation for GENERIC_KEYPAIRS #611

Merged
merged 3 commits into from
Oct 26, 2023
Merged

el2g: add documentation for GENERIC_KEYPAIRS #611

merged 3 commits into from
Oct 26, 2023

Conversation

arnout
Copy link
Contributor

@arnout arnout commented Oct 6, 2023

lmp-el2go-auto-register was extended with support for generic keypairs in PR foundriesio/meta-lmp#1307. Document this new feature, in particular that GENERIC_KEYPAIRS has to be set in /etc/default/lmp-el2go-auto-register.

Readiness

  • Merge (pending reviews)
  • Merge after date or event
  • Draft

Overview

Why merge this PR? What does it solve?

Checklist

  • Run spelling and grammar check, preferably with linter.
  • Avoid changing any header associated with a link/reference.
  • Step through instructions (or ask someone to do so).
  • Review for wordiness
  • Match tone and style of page/section.
  • Run make linkcheck.
  • View HTML in a browser to check rendering.
  • Use semantic newlines.
  • follow best practices for commits.
    • Descriptive title written in the imperative.
    • Include brief overview of QA steps taken.
    • Mention any related issues numbers.
    • End message with sign off/DCO line (-s, --signoff).
    • Sign commit with your gpg key (-S, --gpg-sign).
    • Squash commits if needed.
  • Request PR review by a technical writer and at least one peer.

Comments

Any thing else that a maintainer/reviewer should know.
This could include potential issues, rational for approach, etc.

@arnout
el2g: add documentation for GENERIC_KEYPAIRS
5ab8290 
lmp-el2go-auto-register was extended with support for generic keypairs.
Document this new feature, in particular that GENERIC_KEYPAIRS has to be
set in /etc/default/lmp-el2go-auto-register.

Signed-off-by: Arnout Vandecappelle <arnout.vandecappelle@mind.be>
Signed-off-by: Arnout Vandecappelle <arnout.vandecappelle@fortrobotics.com>
@arnout arnout mentioned this pull request Oct 6, 2023
Merged
@kprosise kprosise requested review from a team, doanac and angolini October 9, 2023 14:34
kprosise
kprosise reviewed Oct 9, 2023
View reviewed changes
Copy link
Contributor

@kprosise kprosise left a comment

Choose a reason for hiding this comment

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

Left some minor suggestions, otherwise it looks good to me. Thanks for this!

source/user-guide/el2g.rst Outdated Show resolved Hide resolved
source/user-guide/el2g.rst Outdated Show resolved Hide resolved
source/user-guide/el2g.rst Show resolved Hide resolved
source/user-guide/el2g.rst Outdated Show resolved Hide resolved
source/user-guide/el2g.rst Outdated Show resolved Hide resolved
angolini
angolini approved these changes Oct 10, 2023
View reviewed changes
Copy link
Collaborator

@angolini angolini left a comment

Choose a reason for hiding this comment

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

Only some minor suggestions.

source/user-guide/el2g.rst Outdated
Installing additional Secure Objects
------------------------------------

Any additional Secure Objects that are defined in EdgeLock 2GO and provisioned into the secure element when the device registers.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This sentence is a bit off to me. I think the purpose here is to say that any additional secure objects <....> can be installed, or maybe list which are the expected additional objects. I think something is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The and should be are. But the sentence is still off. How about:

Additional Secure Objects can be defined in EdgeLock 2GO through the API.
These objects are provisioned into the Secure Element when the device registers.

source/user-guide/el2g.rst Outdated Show resolved Hide resolved
@arnout @kprosise
Apply suggestions from code review
f98943a 
Co-authored-by: Katrina Prosise <katrina.prosise@foundries.io>
arnout
arnout commented Oct 11, 2023
View reviewed changes
Copy link
Contributor Author

@arnout arnout left a comment

Choose a reason for hiding this comment

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

I applied all suggested changes but have questions about the other comments.

source/user-guide/el2g.rst Outdated
Installing additional Secure Objects
------------------------------------

Any additional Secure Objects that are defined in EdgeLock 2GO and provisioned into the secure element when the device registers.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The and should be are. But the sentence is still off. How about:

Additional Secure Objects can be defined in EdgeLock 2GO through the API.
These objects are provisioned into the Secure Element when the device registers.

source/user-guide/el2g.rst Outdated Show resolved Hide resolved
source/user-guide/el2g.rst Outdated Show resolved Hide resolved
doanac
doanac approved these changes Oct 16, 2023
View reviewed changes
Copy link
Member

@doanac doanac left a comment

Choose a reason for hiding this comment

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

Thanks @arnout !

kprosise
kprosise approved these changes Oct 20, 2023
View reviewed changes
Copy link
Contributor

@kprosise kprosise left a comment

Choose a reason for hiding this comment

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

Just a couple suggestions related to other feedback.

source/user-guide/el2g.rst Outdated Show resolved Hide resolved
source/user-guide/el2g.rst Outdated Show resolved Hide resolved
@arnout @kprosise
Apply suggestions from code review
5105319 
Co-authored-by: Katrina Prosise <katrina.prosise@foundries.io>
@kprosise kprosise merged commit 207c66f into foundriesio:main Oct 26, 2023
1 of 3 checks passed
@arnout
Copy link
Contributor Author

arnout commented Oct 26, 2023

Was it a good idea to merge this already? It documents a feature added in foundriesio/meta-lmp#1307 and that hasn't been merged yet (or received any feedback, actually).

@doanac
Copy link
Member

doanac commented Oct 26, 2023

Was it a good idea to merge this already?

It will be okay. We can poke @rsalveti to merge it soonish - and it will all get released at the same time when the next LmP comes out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@doanac doanac doanac approved these changes

@angolini angolini angolini approved these changes

@kprosise kprosise kprosise approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@arnout @doanac @angolini @kprosise