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

overlay/15fcos: coreos-check-ssh-keys: limit search to current boot #1373

Merged
merged 1 commit into from
Dec 17, 2021

Conversation

aaradhak
Copy link
Member

fixed the problem by limiting the journal output to the most recent boot - jira #1033

@bgilbert
Copy link
Contributor

Code LGTM. This could probably use a comment explaining why we're explicitly checking only the current boot.

Similarly, let's summarize the problem being fixed in the commit message. (Also, the correct link is coreos/fedora-coreos-tracker#1033, and it's not a Jira card. 🙂)

@dustymabe
Copy link
Member

code LGTM as well. Agree with the points benjamin made. Also might want to change the commit title to something like:

overlay/15fcos: coreos-check-ssh-keys: limit search to current boot

@aaradhak aaradhak changed the title "Fix for duplicate Ignition message problem" overlay/15fcos: coreos-check-ssh-keys: limit search to current boot Dec 16, 2021
@aaradhak
Copy link
Member Author

Hi @bgilbert @dustymabe , I have made the changed you had mentioned in the comments. Kindly provide your feedback.

Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

hey @aaradhak. The commit message looks great. A few improvements:

  • remove the quotation marks in the commit title
  • break up lines at around 70 to 80 characters
  • the last line should be something like Fixes https://github.com/coreos/fedora-coreos-tracker/issues/1033

Also as bgilbert said. It would be good if we added this context as a comment in the code. I made the suggestion inline in the code to show you what we mean.

overlay.d/15fcos/usr/libexec/coreos-check-ssh-keys Outdated Show resolved Hide resolved
overlay.d/15fcos/usr/libexec/coreos-check-ssh-keys Outdated Show resolved Hide resolved
@aaradhak aaradhak force-pushed the fedoraduar branch 2 times, most recently from bcdbfb7 to 2155f2f Compare December 16, 2021 23:25
@dustymabe
Copy link
Member

LGTM - let's squash these down to a single commit.

On re-provisioning a FCOS instance, Ignition will append notices like
"wrote ssh authorized keys file for user: %s"
to "/etc/issue.d/30_ssh_authorized_keys.issue".

The problem is that the journal from the previous install remains available
and multiple entries are displayed when the journal is searched.
The problem is fixed by limiting the journal output to the most recent boot.

Fixes coreos/fedora-coreos-tracker#1033
@aaradhak
Copy link
Member Author

Yes, the commits are git squashed now.

LGTM - let's squash these down to a single commit.

@dustymabe dustymabe enabled auto-merge (rebase) December 17, 2021 03:02
@dustymabe dustymabe merged commit af7bcc8 into coreos:testing-devel Dec 17, 2021
@aaradhak aaradhak deleted the fedoraduar branch August 1, 2023 13:11
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