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

The --ulimit option now also accepts the name with an RLIMIT_ prefix both upper and lower case. #18078

Merged
merged 1 commit into from
Apr 7, 2023
Merged

The --ulimit option now also accepts the name with an RLIMIT_ prefix both upper and lower case. #18078

merged 1 commit into from
Apr 7, 2023

Conversation

xpahos
Copy link

@xpahos xpahos commented Apr 6, 2023

RLIMIT_* resources constants are converted to the proper format for the go-units module.

Fixes #18077

Does this PR introduce a user-facing change?

None

Signed-off-by: Alexander Gryanko xpahos@gmail.com

@openshift-ci openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Apr 6, 2023
@xpahos xpahos force-pushed the main branch 2 times, most recently from 1437adf to 6dee301 Compare April 6, 2023 04:42
Luap99
Luap99 reviewed Apr 6, 2023
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I just have a small nit for the test error reporting.

pkg/specgenutil/specgenutil_test.go Outdated
Comment on lines 206 to 218
assert.NotNil(t, err, "err is not nil")

_, err = GenRlimits([]string{"RLIMIT_FOO=1:1"})
assert.NotNil(t, err, "err is not nil")

_, err = GenRlimits([]string{"nofile"})
assert.NotNil(t, err, "err is not nil")

_, err = GenRlimits([]string{"nofile=bar"})
assert.NotNil(t, err, "err is not nil")

_, err = GenRlimits([]string{"nofile=bar:buzz"})
assert.NotNil(t, err, "err is not nil")
Copy link
Member

Choose a reason for hiding this comment

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

consider using assert.Error() for these, this will provide a much friendlier error message in case of an failure

pkg/specgenutil/specgenutil_test.go Outdated
}

parsedLimits, err := GenRlimits(lowerCaseLimits)
assert.Nil(t, err, "err is nil")
Copy link
Member

Choose a reason for hiding this comment

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

here as well, use assert.NoError()

pkg/specgenutil/specgenutil_test.go Outdated
}

parsedLimits, err = GenRlimits(upperCaseLimitsWithPrefix)
assert.Nil(t, err, "err is nil")
Copy link
Member

Choose a reason for hiding this comment

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

same here

@Luap99
Copy link
Member

Luap99 commented Apr 6, 2023

You also need to fix the linting error, make sure to run gofmt on the file.

@Luap99
Copy link
Member

Luap99 commented Apr 6, 2023

Also consider adding a proper release note entry to your PR description, how about?

The `--ulimit` option now also accepts the name with an `RLIMIT_` prefix both upper and lower case.

@xpahos
Copy link
Author

xpahos commented Apr 6, 2023

You also need to fix the linting error, make sure to run gofmt on the file.

Sorry for the stupid question. Maybe you can help me with the local linter? I have tried make validate and podman build -t gate -f contrib/gate/Dockerfile but of them are not working. The first command shows a lot of errors. Docker file doesn't exist.

@xpahos xpahos changed the title Trim RLIMIT_ prefix in GenRlimits before parsing resources The --ulimit option now also accepts the name with an RLIMIT_ prefix both upper and lower case. Apr 6, 2023
@Luap99
Copy link
Member

Luap99 commented Apr 6, 2023

make validate should just work, it does at least on my system.
For contrib/gate/Dockerfile I think we removed this a while ago, looks like we did not update the docs in all places.

@xpahos
Copy link
Author

xpahos commented Apr 6, 2023 •
edited

make validate should just work, it does at least on my system. For contrib/gate/Dockerfile I think we removed this a while ago, looks like we did not update the docs in all places.

I've got a long list of errors https://pastebin.com/7WNY5GCy

go version go1.19.3 linux/amd64

@Luap99
Copy link
Member

Luap99 commented Apr 6, 2023

Oh yeah ignore the typecheck errors, I never figured out why or when it happens but I have seen these before.
If CI is happy then you are good.

The --ulimit option accepts the name with an RLIMIT_ prefix both …
db9785b 
…upper and lower case

Signed-off-by: Alexander Gryanko <xpahos@yandex-team.ru>
@xpahos
Copy link
Author

xpahos commented Apr 6, 2023 •
edited

@Luap99 looks like I need more help. I'm trying to reproduce failed check in the qemu environment. make localsystem returns no errors in the aarch64 emulator.

giuseppe
giuseppe approved these changes Apr 6, 2023
View reviewed changes
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 6, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, xpahos

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 6, 2023
@rhatdan rhatdan added release-note-none and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Apr 6, 2023
@rhatdan
Copy link
Member

rhatdan commented Apr 6, 2023

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 6, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2023
@rhatdan
Copy link
Member

rhatdan commented Apr 7, 2023

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 7, 2023
@openshift-merge-robot openshift-merge-robot merged commit 6ea703b into containers:main Apr 7, 2023
69 checks passed
@edsantiago edsantiago mentioned this pull request Apr 12, 2023
Closed
@witten witten mentioned this pull request Apr 30, 2023
Draft
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 2, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 left review comments

@giuseppe giuseppe giuseppe approved these changes

Assignees

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Problems with RLIMIT_* resources
5 participants
@xpahos @Luap99 @rhatdan @giuseppe @openshift-merge-robot