Add support for containers.conf by rhatdan · Pull Request #2011 · containers/buildah

rhatdan · 2019-12-07T10:59:22Z

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

rh-atomic-bot · 2019-12-16T21:34:33Z

☔ The latest upstream changes (presumably #2025) made this pull request unmergeable. Please resolve the merge conflicts.

rh-atomic-bot · 2019-12-17T16:00:53Z

☔ The latest upstream changes (presumably #2026) made this pull request unmergeable. Please resolve the merge conflicts.

rh-atomic-bot · 2019-12-22T16:07:03Z

☔ The latest upstream changes (presumably 6941254) made this pull request unmergeable. Please resolve the merge conflicts.

QiWang19 · 2020-01-03T05:31:22Z

pkg/cli/common.go

 	return fs
 }

+func SetDefaultConfig(cmd *cobra.Command, defaultConfig *config.Config) error {


Do not need the function SetDefaultConfig because gloabl option --containers-conf was removed.

vrothberg · 2020-01-07T10:54:03Z

tests/containers.conf

@@ -0,0 +1,368 @@
+# The containers configuration file specifies all of the available configuration


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.

Yes maybe we should remove all of the commented out ones.

vrothberg · 2020-01-07T10:54:43Z

pkg/cli/common.go

 }
+
+// GetCapabilities returns the capabilities added to container
+func GetCapabilities(defaultCapabilities, addCapabilities, dropCapabilities []string) []string {


s/GetCapabilities/Capabilities/ ... go conventionally doesn't use getter or setter identifiers.

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.

vrothberg · 2020-01-07T10:56:41Z

cmd/buildah/common.go

 `
 }
+
+func getDefaultConfig() *config.Config {


s/getDefaultConfig/defaultConfig/

vrothberg · 2020-01-07T10:58:15Z

cmd/buildah/common.go

+	defaultConfig, err := config.NewConfig("")
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "%v\n", err)
+		os.Exit(1)


I prefer setting defaultConfig in the main.go as I think it's more idiomatic (non-blocking).

vrothberg · 2020-01-07T10:59:13Z

cmd/buildah/common.go


-var needToShutdownStore = false
+var (
+	defaultConfig       = getDefaultConfig()


s/defaultConfig/defaultContainersConfig/ ? A comment would be nice for package-global variables.

vrothberg · 2020-01-07T11:02:59Z

tests/containers_conf.bats

@@ -0,0 +1,92 @@
+#!/usr/bin/env bats


💯 for the tests!

vrothberg · 2020-01-07T11:03:16Z

tests/containers_conf.bats

+
+@test "containers.conf selinux test" {
+    if ! which selinuxenabled > /dev/null 2> /dev/null ; then
+        skip "No selinuxenabled"


s/selinuxenabled/selinux enabled/

That is saying the selinuxenabled program does not exists. So it should stay as a single name. I will add "No selinuxenabled application"

vrothberg · 2020-01-09T09:54:47Z

"not ok 214 containers.conf additional devices test" see https://travis-ci.org/containers/buildah/jobs/634359720#L1031

vrothberg · 2020-01-09T09:55:20Z

# ls: /dev/fuse1: No such file or directory
# [ rc=1 (** EXPECTED 0 **) ]
# #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
# #| FAIL: exit code is 1; expected 0
# #\^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

rh-atomic-bot · 2020-01-13T22:40:04Z

☔ The latest upstream changes (presumably 720e5d6) made this pull request unmergeable. Please resolve the merge conflicts.

rhatdan · 2020-01-14T15:06:37Z

@vrothberg Now that I have this testing, can I bump containers/common version so we can merge this?

vrothberg · 2020-01-14T15:09:37Z

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

rhatdan · 2020-01-14T19:45:23Z

Since we are still a version 0, I don't believe this is true. We are still working towards a stable release.

vrothberg · 2020-01-15T05:04:52Z

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>

vrothberg

LGTM (there are some older nits but those are non-blocking)

vrothberg · 2020-01-15T12:55:41Z

Nice work, @QiWang19 and @rhatdan !

TomSweeneyRedHat · 2020-01-15T16:00:03Z

LGTM.
I ran into some issues testing the changes with one of the baseline tests, but have convinced myself it's not due to this change. Most likely a system error.

rhatdan · 2020-01-15T17:24:31Z

@rh-atomic-bot r=TomSweeneyRedHat

rh-atomic-bot · 2020-01-15T17:24:32Z

📌 Commit 798ebd8 has been approved by TomSweeneyRedHat

rh-atomic-bot · 2020-01-15T17:24:40Z

⚡ Test exempted: pull fully rebased and already tested.

rhatdan changed the title ~~Add support for containers.conf~~ [WIP] Add support for containers.conf Dec 7, 2019

rhatdan force-pushed the containers.conf branch 6 times, most recently from c0979b6 to 13ddc21 Compare December 13, 2019 10:40

rhatdan force-pushed the containers.conf branch from 13ddc21 to 49e22ae Compare December 16, 2019 21:43

rhatdan force-pushed the containers.conf branch 2 times, most recently from 566bcac to 23e44ee Compare December 17, 2019 16:08

rhatdan force-pushed the containers.conf branch from 23e44ee to 41dd9fb Compare December 23, 2019 11:08

rhatdan force-pushed the containers.conf branch from 41dd9fb to a71aedb Compare January 2, 2020 20:19

QiWang19 reviewed Jan 3, 2020

View reviewed changes

rhatdan force-pushed the containers.conf branch from a71aedb to 9c75d83 Compare January 3, 2020 19:35

rhatdan changed the title ~~[WIP] Add support for containers.conf~~ Add support for containers.conf Jan 3, 2020

rhatdan force-pushed the containers.conf branch 4 times, most recently from d7fa14f to 7452459 Compare January 3, 2020 21:58

rhatdan changed the title ~~Add support for containers.conf~~ [WIP] Add support for containers.conf Jan 3, 2020

rhatdan force-pushed the containers.conf branch from 7452459 to 4569fae Compare January 6, 2020 14:20

vrothberg reviewed Jan 7, 2020

View reviewed changes

rhatdan force-pushed the containers.conf branch 3 times, most recently from 6823a42 to c390a19 Compare January 8, 2020 18:05

rhatdan force-pushed the containers.conf branch from c390a19 to ba947f6 Compare January 14, 2020 12:32

rhatdan force-pushed the containers.conf branch from ba947f6 to 798ebd8 Compare January 15, 2020 12:06

rhatdan changed the title ~~[WIP] Add support for containers.conf~~ Add support for containers.conf Jan 15, 2020

vrothberg reviewed Jan 15, 2020

View reviewed changes

rh-atomic-bot closed this in 09d1c24 Jan 15, 2020

nalind mentioned this pull request Feb 17, 2020

PATH being reset #2158

Closed

TomSweeneyRedHat mentioned this pull request Jan 14, 2021

Bump to v1.19.1 #2912

Closed

github-actions bot added the locked - please file new issue/PR label Oct 6, 2023

github-actions bot locked as resolved and limited conversation to collaborators Oct 6, 2023

		@@ -0,0 +1,368 @@
		# The containers configuration file specifies all of the available configuration

Conversation

rhatdan commented Dec 7, 2019

Uh oh!

rh-atomic-bot commented Dec 16, 2019

Uh oh!

rh-atomic-bot commented Dec 17, 2019

Uh oh!

rh-atomic-bot commented Dec 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrothberg commented Jan 9, 2020

Uh oh!

vrothberg commented Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rh-atomic-bot commented Jan 13, 2020

Uh oh!

rhatdan commented Jan 14, 2020

Uh oh!

vrothberg commented Jan 14, 2020

Uh oh!

rhatdan commented Jan 14, 2020

Uh oh!

vrothberg commented Jan 15, 2020 via email

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

vrothberg commented Jan 15, 2020

Uh oh!

TomSweeneyRedHat commented Jan 15, 2020

Uh oh!

rhatdan commented Jan 15, 2020

Uh oh!

rh-atomic-bot commented Jan 15, 2020

Uh oh!

rh-atomic-bot commented Jan 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vrothberg commented Jan 9, 2020 •

edited

Loading