Skip to content

Add cmd/config/config.go#41

Merged
haircommander merged 2 commits intocontainers:masterfrom
haircommander:go-config
Jun 13, 2019
Merged

Add cmd/config/config.go#41
haircommander merged 2 commits intocontainers:masterfrom
haircommander:go-config

Conversation

@haircommander
Copy link
Collaborator

Now, we can configure conmon from within the conmon directory.

This will allow tools to vendor conmon and import the values they need, rather than having the configuration parameters in different repositories

This PR also comes with the eventual goal of coupling containers/conmon with the go code that calls it, so libraries that use conmon don't have to worry too much about conflicting versions

Signed-off-by: Peter Hunt pehunt@redhat.com

@haircommander haircommander force-pushed the go-config branch 6 times, most recently from 9bcade6 to 4dbdb9b Compare June 12, 2019 13:49
@haircommander
Copy link
Collaborator Author

I added a CI check for a dirty config (one that was changed in oci/oci_config* but the config wasn't added to the PR). PTAL @cevich

@haircommander haircommander force-pushed the go-config branch 4 times, most recently from f0bae8b to 303e63d Compare June 12, 2019 14:08
Now, we can configure conmon from within the conmon repository.
This will allow tools to vendor conmon and import the values they need, rather than having the configuration parameters in different repositories
Also added runner/ package to store the configuration parameters

Signed-off-by: Peter Hunt <pehunt@redhat.com>
@haircommander
Copy link
Collaborator Author

@saschagrunert @rhatdan @mheon PTAL

@haircommander haircommander requested a review from cevich June 12, 2019 16:04
@mheon
Copy link
Member

mheon commented Jun 12, 2019

The C and Go look fine. I can't comment on the testing YAML or bash bits.

@cevich
Copy link
Member

cevich commented Jun 12, 2019

looking...

Copy link
Member

@cevich cevich left a comment

Choose a reason for hiding this comment

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

Looks good, just needs to re-use the gating container, and fixup the dependencies/depends_on.

@haircommander haircommander force-pushed the go-config branch 2 times, most recently from a42e2b2 to ee47cb0 Compare June 12, 2019 17:10
@cevich
Copy link
Member

cevich commented Jun 12, 2019

Ugggg....my wires are totally crossed, taking a fresh look...

@cevich
Copy link
Member

cevich commented Jun 12, 2019

Okay, remembering now this is actually the conmon repo. 😄 I think my maintenance concern is still valid, let's see if this can be made more "DRY" (untested - not sure this will work):

diff --git a/.cirrus.yml b/.cirrus.yml
index 07c7b1c..3c9fb59 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -20,9 +20,11 @@ env:
     TRAVIS: "true"
 
     ####
-    #### Cache-image names to test with
+    #### Image names to test with
     ###
     FEDORA_CACHE_IMAGE_NAME: 'fedora-29-conmon-75ea13be'
+    FEDOTA_CONTAINER_FQIN: 'registry.fedoraproject.org/fedora:29'
+    PRIOR_FEDORA_CONTAINER_FQIN: 'registry.fedoraproject.org/fedora:28'
 
     ####
     #### Variables for composing new cache-images (used in PR testing) from
@@ -63,6 +65,9 @@ timeout_in: '120m'
 # testing for every platform
 integration_task:
 
+    depends_on:
+        - 'config'
+
     gce_instance:
         # Generate multiple parallel tasks, covering all possible
         # 'matrix' combinations.
@@ -85,8 +90,8 @@ fedora_packaging_task:
     # Runs within Cirrus's "community cluster"
     container:
         matrix:
-            image: "registry.fedoraproject.org/fedora:28"
-            image: "registry.fedoraproject.org/fedora:29"
+            image: "${FEDOTA_CONTAINER_FQIN}"
+            image: "${PRIOR_FEDORA_CONTAINER_FQIN}"
         cpu: 4
         memory: 12
 
@@ -111,7 +116,7 @@ config_task:
                # fedora:28 doesn't have go mod by default
                # and we should only need one check to make sure
                # config changes were synced
-            image: "registry.fedoraproject.org/fedora:29"
+            image: "${FEDOTA_CONTAINER_FQIN}"
         cpu: 4
         memory: 12
 

@cevich
Copy link
Member

cevich commented Jun 12, 2019

Reason I suggest making 'integration' depend on 'config is: "Fail fast". But I'm not actually sure if the config test failing would make the integration test results invalid or not.

@cevich
Copy link
Member

cevich commented Jun 12, 2019

Otherwise, this LGTM 😄

@haircommander
Copy link
Collaborator Author

haircommander commented Jun 12, 2019

let's try it out :D (it could make integration tests invalid, and also we'd spend money on spin cycles that would fail anyway)

@cevich
Copy link
Member

cevich commented Jun 12, 2019

ya, z'ktly, and clearly there are no actual gating tests here, so it's a start.

@cevich
Copy link
Member

cevich commented Jun 12, 2019

Neat it works, okay, well there ya go! Sorry for my confusion...it must have been very confusing 😁

@haircommander
Copy link
Collaborator Author

haircommander commented Jun 12, 2019

(testing to verify it stops jobs if config fails)

edit: works as expected :)

Signed-off-by: Peter Hunt <pehunt@redhat.com>
@haircommander
Copy link
Collaborator Author

@saschagrunert I'm tagging you to merge this if it looks good to you :)

Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

Just one nit but this seems no blocker right now 👇

@haircommander haircommander merged commit ba3e4dc into containers:master Jun 13, 2019
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.

4 participants