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

Commit to separate Dracut #437

Merged
merged 2 commits into from
Jan 17, 2024
Merged

Commit to separate Dracut #437

merged 2 commits into from
Jan 17, 2024

Conversation

gb-123-git
Copy link

This Commit is based on the work of @sergio-correia & @jpds. I have just rebased it with current tree.
Should close #347 & #346.

Hope this gets merged quickly.

@gb-123-git
Copy link
Author

gb-123-git commented Aug 14, 2023

@sergio-correia @jpds @Anonymous1157

I have re-based the same files over the current tree. Can you guys check and test it (for Alpine Linux too please) ?

@gb-123-git
Copy link
Author

gb-123-git commented Aug 14, 2023

@sergio-correia Anyway this can be merged to a new branch so that there can be more changes by maintainers / other developers etc. and later when everything is ok, it can be merged with the main branch ? Anyway I can give you write access to my fork ?

@gb-123-git
Copy link
Author

@sergio-correia
Please let me know if anything else needs to be done. Made the changes you suggested.
Thanks

@sergio-correia
Copy link
Collaborator

@sergio-correia Please let me know if anything else needs to be done. Made the changes you suggested. Thanks

Thanks, I will do a more thorough review and actual testing soon and will report back.

@sarroutbi
Copy link
Collaborator

Could you please review issues reported by shellcheck? I guess that, at least, next can be removed:

Error: SHELLCHECK_WARNING:
./src/dracut/clevis-pin-tpm2/module-setup.sh.in:39:26: error[SC2283]: Remove spaces around = to assign (or use [ ] to compare, or quote '=' if literal).

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.

Changes in general LGTM. Please, review comments provided in code

@gb-123-git
Copy link
Author

gb-123-git commented Aug 24, 2023

@sarroutbi

Error: SHELLCHECK_WARNING:
./src/dracut/clevis-pin-tpm2/module-setup.sh.in:39:26: error[SC2283]: Remove spaces around = to assign (or use [ ] to compare, or quote '=' if literal).

The above has been resolved.

For the other issues i.e. 'variables assigned but not used', I would require help of @sergio-correia & @jpds who are the actual author of the code.

Update: Some of the variables are actually coming from command line so I think some of them are just false warnings. @jpds & @sergio-correia would be able to comment better.

gb-123-git

This comment was marked as resolved.

@gb-123-git
Copy link
Author

@sarroutbi @sergio-correia
Any progress on merging this ?
Anything I need to do ?

@sarroutbi
Copy link
Collaborator

@sarroutbi @sergio-correia Any progress on merging this ? Anything I need to do ?

Hello. Did you have a chance to check latest Code scanning/shellcheck issues?

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

Do these values need to be exported?

@gb-123-git
Copy link
Author

@sarroutbi @sergio-correia Any progress on merging this ? Anything I need to do ?

Hello. Did you have a chance to check latest Code scanning/shellcheck issues?

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

Do these values need to be exported?

Some of the variables are actually coming from command line and passed on (eg 'hostonly' option) to dracut so I think they are just false warnings. @jpds & @sergio-correia would be able to comment better.

@gb-123-git
Copy link
Author

@sergio-correia @jpds
Can you guys please help fix this so it gets merged before it needs another re-base ?

@gb-123-git
Copy link
Author

@sergio-correia & @jpds

Just another humble request to help me merge this ? Thanks!

@sergio-correia
Copy link
Collaborator

@sergio-correia & @jpds

Just another humble request to help me merge this ? Thanks!

Apologies for the delay. I intend to test it this week.

@gb-123-git
Copy link
Author

@sergio-correia - Just checking if there are any updates ? Did you get the time to check ? Thanks

@gb-123-git
Copy link
Author

@sergio-correia - Any update ?

@sergio-correia
Copy link
Collaborator

I got stuck with testing this, I will try to complete it ASAP.

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.

Changes LGTM

Prepare to decouple dracut and systemd unlockers.
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 comment. Rest of changes LGTM

src/luks/dracut/clevis/clevis-luks-unlocker Show resolved Hide resolved
Add an unlocker that does not require systemd.
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 sergio-correia merged commit afe91eb into latchset:master Jan 17, 2024
10 of 11 checks passed
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.

3 participants