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
Add support for containers.conf #2011
Conversation
c0979b6
to
13ddc21
Compare
☔ The latest upstream changes (presumably #2025) made this pull request unmergeable. Please resolve the merge conflicts. |
13ddc21
to
49e22ae
Compare
☔ The latest upstream changes (presumably #2026) made this pull request unmergeable. Please resolve the merge conflicts. |
566bcac
to
23e44ee
Compare
☔ The latest upstream changes (presumably 6941254) made this pull request unmergeable. Please resolve the merge conflicts. |
23e44ee
to
41dd9fb
Compare
41dd9fb
to
a71aedb
Compare
pkg/cli/common.go
Outdated
@@ -220,6 +226,45 @@ func GetFromAndBudFlags(flags *FromAndBudResults, usernsResults *UserNSResults, | |||
return fs | |||
} | |||
|
|||
func SetDefaultConfig(cmd *cobra.Command, defaultConfig *config.Config) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not need the function SetDefaultConfig
because gloabl option --containers-conf
was removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
a71aedb
to
9c75d83
Compare
d7fa14f
to
7452459
Compare
7452459
to
4569fae
Compare
tests/containers.conf
Outdated
@@ -0,0 +1,368 @@ | |||
# The containers configuration file specifies all of the available configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a bit hard to maintain and understand as some options are commented out while others are not but it's very hard to figure out which are actually used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes maybe we should remove all of the commented out ones.
pkg/cli/common.go
Outdated
@@ -279,3 +281,32 @@ func CheckAuthFile(authfile string) error { | |||
} | |||
return nil | |||
} | |||
|
|||
// GetCapabilities returns the capabilities added to container | |||
func GetCapabilities(defaultCapabilities, addCapabilities, dropCapabilities []string) []string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/GetCapabilities/Capabilities/ ... go conventionally doesn't use getter or setter identifiers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this func a potential candidate for c/common ? I guess that's something libpod needs as well. Based on an earlier comment from @mheon in c/common I am on the lookout for code we could further consolidate in c/common.
@@ -424,3 +428,13 @@ Flags: | |||
{{end}} | |||
` | |||
} | |||
|
|||
func getDefaultConfig() *config.Config { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/getDefaultConfig/defaultConfig/
defaultConfig, err := config.NewConfig("") | ||
if err != nil { | ||
fmt.Fprintf(os.Stderr, "%v\n", err) | ||
os.Exit(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer setting defaultConfig
in the main.go
as I think it's more idiomatic (non-blocking).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
cmd/buildah/common.go
Outdated
@@ -20,7 +21,10 @@ import ( | |||
"github.com/spf13/pflag" | |||
) | |||
|
|||
var needToShutdownStore = false | |||
var ( | |||
defaultConfig = getDefaultConfig() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/defaultConfig/defaultContainersConfig/ ? A comment would be nice for package-global variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -0,0 +1,92 @@ | |||
#!/usr/bin/env bats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 for the tests!
tests/containers_conf.bats
Outdated
|
||
@test "containers.conf selinux test" { | ||
if ! which selinuxenabled > /dev/null 2> /dev/null ; then | ||
skip "No selinuxenabled" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/selinuxenabled/selinux enabled/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is saying the selinuxenabled program does not exists. So it should stay as a single name. I will add "No selinuxenabled application"
6823a42
to
c390a19
Compare
"not ok 214 containers.conf additional devices test" see https://travis-ci.org/containers/buildah/jobs/634359720#L1031 |
|
☔ The latest upstream changes (presumably 720e5d6) made this pull request unmergeable. Please resolve the merge conflicts. |
c390a19
to
ba947f6
Compare
@vrothberg Now that I have this testing, can I bump containers/common version so we can merge this? |
@rhatdan as long as we strictly follow semantic versioning, sure. Any breaking change would mean a major version bump. |
Since we are still a version 0, I don't believe this is true. We are still working towards a stable release. |
I reread through semver.org and you’re right. Everything below 1.0.0 is a
free ticket to do whatever we want 👌
…On Tue 14. Jan 2020 at 20:45, Daniel J Walsh ***@***.***> wrote:
Since we are still a version 0, I don't believe this is true. We are still
working towards a stable release.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2011?email_source=notifications&email_token=ACZDRA5KDASBFUGKQEGOJJDQ5YI5HA5CNFSM4JXLL4Z2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEI536AQ#issuecomment-574340866>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACZDRA3WOO6ZMML7GZDN773Q5YI5HANCNFSM4JXLL4ZQ>
.
|
This is a rework of Qi Wang's patches. Import package pkg/config from containers/common to read containers.conf This patch allows users to specify default values stored in containers.conf that will modify the behaviour of buildah tool. Signed-off-by: Qi Wang <qiwan@redhat.com> Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
ba947f6
to
798ebd8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (there are some older nits but those are non-blocking)
LGTM. |
@rh-atomic-bot r=TomSweeneyRedHat |
📌 Commit 798ebd8 has been approved by |
⚡ Test exempted: pull fully rebased and already tested. |
This is a rework of Qi Wang's patches.
Import package pkg/config from containers/common to read containers.conf
This patch allows users to specify default values stored in containers.conf
that will modify the behaviour of buildah tool.
Signed-off-by: Qi Wang qiwan@redhat.com
Signed-off-by: Daniel J Walsh dwalsh@redhat.com
Replaces #1953