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

clean up a bit of the installer docs #4187

Merged
merged 1 commit into from
Aug 30, 2024
Merged

Conversation

deitch
Copy link
Contributor

@deitch deitch commented Aug 30, 2024

Just a little bit of installer docs cleanup.

@deitch deitch requested a review from eriknordmark as a code owner August 30, 2024 09:02
@deitch
Copy link
Contributor Author

deitch commented Aug 30, 2024

Should not require full CI other than yetus, since I am changing just two markdown files.

@deitch
Copy link
Contributor Author

deitch commented Aug 30, 2024

I am ignoring yetus complaints about sections I did not change.

@christoph-zededa
Copy link
Contributor

I am ignoring yetus complaints about sections I did not change.

But don't they appear because of your changes?
I mean f.e.:

markdownlint:MD051/link-fragments Link fragments should be valid [Context: "[following these instructions](#how-to-write-eve-image-and-installer-onto-an-sd-card-or-an-installer-medium)"]

and you changed the "location" so that they point into the void.

@deitch
Copy link
Contributor Author

deitch commented Aug 30, 2024

Oh, you may be right. I just looked at the line it was complaining about and thought, "there goes yetus again". Good catch @christoph-zededa ; I will check and update if necessary

@deitch
Copy link
Contributor Author

deitch commented Aug 30, 2024

Yup, you were right. Fixed it.

@deitch
Copy link
Contributor Author

deitch commented Aug 30, 2024

This is strange. How am I seeing go test failures for something that changes only markdown? This markdown cannot have broken go code.

I also see it is complaining about lint messages. The message is brief and to the point, and is signed. What is it complaining about?

@OhmSpectator
Copy link
Member

I also see it is complaining about lint messages. The message is brief and to the point, and is signed. What is it complaining about?

We would like to see a body for commit messages. Like what you usually put into the PR description =)

@OhmSpectator
Copy link
Member

This is strange. How am I seeing go test failures for something that changes only markdown? This markdown cannot have broken go code.

ERROR: failed to solve: lfedge/eve-alpine:1f7685f95a475c6bbe682f0b976f12180b6c8726: failed to resolve source metadata for docker.io/lfedge/eve-alpine:1f7685f95a475c6bbe682f0b976f12180b6c8726: failed to authorize: failed to fetch oauth token: unexpected status from POST request to https://auth.docker.io/token: 502 Bad Gateway

I see it time to time...

@deitch
Copy link
Contributor Author

deitch commented Aug 30, 2024

We would like to see a body for commit messages. Like what you usually put into the PR description

It shouldn't complain about a single line; sometimes that is more than enough.

@OhmSpectator
Copy link
Member

It shouldn't complain about a single line; sometimes that is more than enough.

Sometimes, yes. But in most cases, it is not, and people just don't put any details in commit messages (at the same time, they are fine with providing an explanation in the PR description)).
So we have introduced a quick check to remind devs to put more description.
If you are absolutely sure one line is enough, then the checks can be just ignored. But it's kind of 10% of all cases when people omit body. =(

Copy link
Contributor

@europaul europaul left a comment

Choose a reason for hiding this comment

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

Just some typos in the docker commands, otherwise LGTM

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Signed-off-by: Avi Deitcher <avi@deitcher.net>
@OhmSpectator
Copy link
Member

If I were the one who created the commit, I would put the following body:

Refined installer documentation for clarity and usability. Updated terminology
to "storage media" throughout. Enhanced instructions on flashing images by
recommending better graphical tools. Added disclaimers for third-party tools.

But I do not insist =D

@rene
Copy link
Contributor

rene commented Aug 30, 2024

It shouldn't complain about a single line; sometimes that is more than enough.

Sometimes, yes. But in most cases, it is not, and people just don't put any details in commit messages (at the same time, they are fine with providing an explanation in the PR description)). So we have introduced a quick check to remind devs to put more description. If you are absolutely sure one line is enough, then the checks can be just ignored. But it's kind of 10% of all cases when people omit body. =(

If I were the one who created the commit, I would put the following body:

Refined installer documentation for clarity and usability. Updated terminology
to "storage media" throughout. Enhanced instructions on flashing images by
recommending better graphical tools. Added disclaimers for third-party tools.

But I do not insist =D

@deitch got a specific case, for this kind of changes, yeah, a single line is enough. However, as @OhmSpectator pointed out (very well) the idea is to avoid commits that changes a lot of stuff without a suitable (to not say any) explanation. It works while reviewing the PR, for a couple of days and/or weeks, but once you need to revisit changes done long time ago (or not so long), it might be painful to quickly understand all the details without a good explanation. IMHO, forbid commit message without a body it's an affordable price to pay consider the benefits... TBH the only drawback I can tell is to enforce put a description for messages that one line would be sufficient, like this PR....

But, since we introduced the check, let's follow it, make exceptions just open the way for more discussion, so in this case is better to remove the check.....

@deitch
Copy link
Contributor Author

deitch commented Aug 30, 2024

Agreed. So let's ignore here. Need an approval and I can merge it in.

@OhmSpectator
Copy link
Member

OhmSpectator commented Aug 30, 2024

Yep, let's approve this, and do not slow down the merging)
For some reason I don't see an approve button..
Upd. Done.

@deitch
Copy link
Contributor Author

deitch commented Aug 30, 2024

It worked, thanks @OhmSpectator

I will merge it in to stop wasting eden test resources on this.

@deitch deitch merged commit b211ae2 into lf-edge:master Aug 30, 2024
34 of 40 checks passed
@deitch deitch deleted the usb-flasher branch August 30, 2024 12:04
@OhmSpectator
Copy link
Member

It worked, thanks @OhmSpectator

I will merge it in to stop wasting eden test resources on this.

By the way, as far as I understand, Eden will work anyway, as a part of "push" into master action. Yep, we have a lot of actions working on commits at the moment they're merged...

@deitch
Copy link
Contributor Author

deitch commented Aug 30, 2024

Good point. Yash is working on all of that as far as I know.

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.

5 participants