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.

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

pin template with documentation, sample "file" pin #203

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cbiedl
Copy link

@cbiedl cbiedl commented May 24, 2020

At least for me, writings pins was hard since I had to figure out the concept behind them, and when studying the existing ones, finding out what's generic pin logic and what it specific for that pin wasn't that easy either.

That should not be repeated for anyone else, so I created src/pins/template/ and filled it my several files and a lot of explanations embedded.

Also a new pin "file" as a demonstration of a minimal pin that uses a file to store the key. I'd consider this suitable for educational puposes only, but you think it's worth to include it in the official distribution, go ahead. Actually, this was im preparation of #185 which will get a fix soon.

This should ease writing new pins a lot, for newcomers and for
experienced authors as well.

== BUGS

Requires the directories for that file already exists.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rewrite this as:
"Requires that directories for that file already exist"

Copy link
Collaborator

@sarroutbi sarroutbi left a comment

Choose a reason for hiding this comment

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

Minor change in documentation suggestion. Rest of changes LGTM.

@cbiedl
Copy link
Author

cbiedl commented Jan 28, 2023

Certainly. Not being a native English speaker, such glitches do happen.
Do you want an updated pull request? Might be less work if you just do it on your own.

@cbiedl cbiedl force-pushed the template-file-pin branch from 08226c6 to 91f9058 Compare January 28, 2023 16:25
src/pins/file/initramfs.in Fixed Show fixed Hide fixed
src/pins/file/pin-file Fixed Show fixed Hide fixed
. @initramfstoolsdir@/hook-functions

die() {
code="$1"

Check warning

Code scanning / shellcheck

code appears unused. Verify use (or export if used externally).

code appears unused. Verify use (or export if used externally).
This is a very simple pin, and possibly rather for educational
purposes: Store the jwk at a given place in the file system.
@cbiedl cbiedl force-pushed the template-file-pin branch from 91f9058 to b44e13e Compare January 28, 2023 16:31
@cbiedl
Copy link
Author

cbiedl commented Jan 28, 2023

Okay, found some minor issues so I've updated my branch (also, rebased it on top of HEAD). This resulted in some warnings/errors from CI I fail to understand. Feel free to take it as-is.

@sarroutbi
Copy link
Collaborator

Okay, found some minor issues so I've updated my branch (also, rebased it on top of HEAD). This resulted in some warnings/errors from CI I fail to understand. Feel free to take it as-is.

Thanks for the PR. The differential shellcheck issue looks like a false positive, so no need to worry for that.

@ghost
Copy link

ghost commented Nov 30, 2023

Is there any way one can assist to getting this merged?

@sarroutbi
Copy link
Collaborator

Changes LGTM. @sergio-correia: can you please provide feedback when possible?

Copy link
Collaborator

@sarroutbi sarroutbi left a comment

Choose a reason for hiding this comment

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

Change LGTM. @sergio-correia : can you please provide your feedback when possible?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants