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

*: fix golangci-lint warnings #1149

Merged
merged 4 commits into from
Dec 20, 2020
Merged

*: fix golangci-lint warnings #1149

merged 4 commits into from
Dec 20, 2020

Conversation

sohankunkerkar
Copy link
Member

Fixes part of #1121
Addresses the following warnings:
https://gist.github.com/sohankunkerkar/e7a8a97be039283d841db527acd7d360

And a few more errcheck warnings which are not covered in this gist, but appeared in an ad-hoc manner while trying to fix these warnings.

bgilbert
bgilbert reviewed Dec 9, 2020
View reviewed changes
internal/exec/stages/files/files_test.go Show resolved Hide resolved
internal/exec/engine.go Outdated
@@ -91,6 +91,7 @@ func (e Engine) Run(stageName string) error {
systemBaseConfig, r, err := system.FetchBaseConfig(e.Logger, e.PlatformConfig.Name())
e.logReport(r)
if err != nil && err != providers.ErrNoProvider {
//nolint:errcheck
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, let's discuss this one OOB.

tests/filesystem.go Outdated
@@ -224,6 +224,7 @@ func setupDisk(ctx context.Context, disk *types.Disk, diskIndex int, imageSize i
loopdev := disk.Device
defer func() {
if err != nil {
//nolint:errcheck
destroyDevice(loopdev)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does something like _ = destroyDevice(loopdev) satisfy the linter? That might be cleaner.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the linter doesn't complain after this change.

internal/exec/stages/disks/luks.go Outdated
@@ -110,7 +110,9 @@ func (s *stage) createLuks(config types.Config) error {
var ignitionCreatedKeyFile bool
// create keyfile inside of tmpfs, it will be copied to the
// sysroot by the files stage
os.MkdirAll(distro.LuksInitramfsKeyFilePath(), 0700)
if err := os.MkdirAll(distro.LuksInitramfsKeyFilePath(), 0700); err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

The surrounding code wraps errors before returning them, so we should do that here too.

internal/exec/util/passwd.go Outdated
"IGNITION_USER_NAME": c.Name,
"IGNITION_PATH": path,
"MESSAGE_ID": ignitionSSHAuthorizedkeysMessageID,
})
}); err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise, should be wrapped. Though I'm not sure we should consider it a fatal error if we can't log a journal message.

Copy link
Member Author

@sohankunkerkar sohankunkerkar Dec 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

How about this one?

		_ = journal.Send(fmt.Sprintf("wrote ssh authorized keys file for user: %s", c.Name), journal.PriInfo, map[string]string{
			"IGNITION_USER_NAME": c.Name,
			"IGNITION_PATH":      path,
			"MESSAGE_ID":         ignitionSSHAuthorizedkeysMessageID,
		})

@sohankunkerkar sohankunkerkar force-pushed the fix-warnings branch 4 times, most recently from 19dcb42 to 8734677 Compare December 11, 2020 03:39
arithx
arithx reviewed Dec 11, 2020
View reviewed changes
internal/exec/stages/files/files_test.go Outdated Show resolved Hide resolved
internal/providers/vmware/vmware_amd64.go Outdated Show resolved Hide resolved
tests/filesystem.go Show resolved Hide resolved
@sohankunkerkar sohankunkerkar force-pushed the fix-warnings branch 2 times, most recently from 6f56b12 to 6ed3271 Compare December 11, 2020 21:04
arithx
arithx approved these changes Dec 15, 2020
View reviewed changes
internal/exec/util/user_group_lookup_test.go Outdated Show resolved Hide resolved
@sohankunkerkar sohankunkerkar linked an issue Dec 15, 2020 that may be closed by this pull request
Fix warnings from golangci-lint #1121
Closed
bgilbert
bgilbert reviewed Dec 19, 2020
View reviewed changes
Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

LGTM generally.

internal/exec/stages/files/files_test.go Outdated Show resolved Hide resolved
@sohankunkerkar sohankunkerkar merged commit 48b95d1 into coreos:master Dec 20, 2020
@sohankunkerkar sohankunkerkar deleted the fix-warnings branch December 20, 2020 23:14
@edcdavid edcdavid mentioned this pull request Oct 24, 2023
Closed
jlebon added a commit to jlebon/ignition that referenced this pull request Oct 25, 2023
@jlebon
stages/disks: retry sgdisk --zap-all invocation
03eb92c 
When `wipeTable` is enabled, we run `sgdisk --zap-all`. But if the table
was corrupted in the first place, `sgdisk` will exit with code 2 which
then breaks boot.

As a workaround, Ignition used to retry the invocation but the context
around it was lost in coreos#544 and coreos#1149 and the retry was removed and
the error-checking was added.

So this patch effectively re-applies 94c98bc ("sgdisk: retry zap-all
operation on failure"), but now with a comment and a test to make sure
we don't regress again.

Closes: coreos/fedora-coreos-tracker#1596
@jlebon jlebon mentioned this pull request Oct 25, 2023
Merged
jlebon added a commit to jlebon/ignition that referenced this pull request Oct 25, 2023
@jlebon
stages/disks: retry sgdisk --zap-all invocation
1434a1a 
When `wipeTable` is enabled, we run `sgdisk --zap-all`. But if the table
was corrupted in the first place, `sgdisk` will exit with code 2 which
then breaks boot.

As a workaround, Ignition used to retry the invocation but the context
around it was lost in coreos#544 and coreos#1149 and the retry was removed and
the error-checking was added.

So this patch effectively re-applies 94c98bc ("sgdisk: retry zap-all
operation on failure"), but now with a comment and a test to make sure
we don't regress again.

Closes: coreos/fedora-coreos-tracker#1596
jlebon added a commit to jlebon/ignition that referenced this pull request Oct 25, 2023
@jlebon
stages/disks: retry sgdisk --zap-all invocation
5717a50 
When `wipeTable` is enabled, we run `sgdisk --zap-all`. But if the table
was corrupted in the first place, `sgdisk` will exit with code 2 which
then breaks boot.

As a workaround, Ignition used to retry the invocation but the context
around it was lost in coreos#544 and coreos#1149 and the retry was removed and
the error-checking was added.

So this patch effectively re-applies 94c98bc ("sgdisk: retry zap-all
operation on failure"), but now with a comment and a test to make sure
we don't regress again.

Closes: coreos/fedora-coreos-tracker#1596
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgwalters cgwalters cgwalters left review comments

@arithx arithx arithx approved these changes

@bgilbert bgilbert bgilbert 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.

Fix warnings from golangci-lint
4 participants
@sohankunkerkar @cgwalters @bgilbert @arithx