Skip to content

local dev/hacking improvements #982

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
Merged
openshift-merge-robot merged 3 commits into from
Jul 19, 2019
Merged

local dev/hacking improvements #982

openshift-merge-robot merged 3 commits into from
Jul 19, 2019

Conversation

@cgwalters
Copy link
Member

@cgwalters cgwalters commented Jul 17, 2019

This fixes the ./hack/cluster.push script to replace all MCO components rather than accepting a WHAT since...a lot of nontrivial changes require doing all of them, and since we have a uni-image we might as well now!

The build-image script changes apply to both the "cluster push fast-path" as well as the "new release image" path.

The HACKING.md doc is updated for the above and more.

cgwalters added 2 commits July 17, 2019 14:28
@cgwalters
hack/cluster-push: Update everything by default
a35abae
@cgwalters
build-image: Rewrite to be based around git hash
2dda0d2 
The high level goal is that I can run `./hack/cluster-push.sh`
and have it automatically do a build only if necessary.

I feel like I'm reinventing something here but...eh.  I don't
want to build the image every time, and I also don't want to run
stale images.  Use the git hash as change detection.  Note this
doesn't properly handle dirty trees yet.
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 17, 2019
@cgwalters
Copy link
Member Author

cgwalters commented Jul 17, 2019

Side note: One thing I struggled with for a bit is that our existing docs encourage pushing to quay.io which you need a secret to do...but having that in your ~/.docker/config.json conflicts with using the OpenShift pull secret to get released containers from quay.io.

Probably for most people with this workflow, it all works fine because your are probably keeping your ~/.docker/config.json separate from the pull secrets used for openshift-installer. But my installer wrapper links these two by automatically injecting the docker secrets which creates a conflict.

For OpenShift developers we could work around this probably by encouraging pushing dev images to registry.svc.ci.openshift.org? Or I guess Docker Hub.

But really I think it's silly to push dev container images to a public registry when really we can push to the in-cluster registry...except what that runs straight up against is the self-signed router by default, which gets into #496 ... it'd really be nice though if we made it super convenient to use Let's Encrypt or so.

kikisdeliveryservice
kikisdeliveryservice reviewed Jul 17, 2019
View reviewed changes
Copy link
Contributor

@kikisdeliveryservice kikisdeliveryservice left a comment

Choose a reason for hiding this comment

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

a couple of comments

hack/build-image Outdated
Copy link
Contributor

@kikisdeliveryservice kikisdeliveryservice Jul 17, 2019

Choose a reason for hiding this comment

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

python!! 😄

@cgwalters
HACKING.md: Update for previous changes
ad89df3 
- Bump Fedora versions
- Drop libvirt stuff for now since most of us aren't using it
  and it's slightly special, may re-add later
- Note to use the 4.2 CI builds

etc.
@cgwalters
Copy link
Member Author

cgwalters commented Jul 17, 2019

Updated the doc per comments

@LorbusChris
Copy link
Contributor

LorbusChris commented Jul 18, 2019

/approve
can you lgtm this, @kikisdeliveryservice ?

@runcom
Copy link
Member

runcom commented Jul 19, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jul 19, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, LorbusChris, runcom

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

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [LorbusChris,cgwalters,runcom]

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

@openshift-merge-robot openshift-merge-robot merged commit 17e53c9 into openshift:master Jul 19, 2019
@cgwalters
Copy link
Member Author

cgwalters commented Jul 19, 2019

Using this some more, I now notice it's doing

--- /tmp/rendered-master-7922d771acbd9d212c63b49ce17ff0b4	2019-07-19 14:46:52.981967525 +0000
+++ /tmp/rendered-master-f3613c0c705e7cd9696b267237be420f	2019-07-19 14:47:10.607957263 +0000
@@ -2,10 +2,10 @@
 kind: MachineConfig
 metadata:
   annotations:
-    machineconfiguration.openshift.io/generated-by-controller-version: 55a73fe074fcec644f93a8abb1c96ea853fecb43
-  creationTimestamp: "2019-07-19T12:39:35Z"
+    machineconfiguration.openshift.io/generated-by-controller-version: 7be16b97ea8a5c1b9035bdea87b2e69561776087
+  creationTimestamp: "2019-07-19T14:40:30Z"
   generation: 1
-  name: rendered-master-7922d771acbd9d212c63b49ce17ff0b4
+  name: rendered-master-f3613c0c705e7cd9696b267237be420f
   ownerReferences:
   - apiVersion: machineconfiguration.openshift.io/v1
     blockOwnerDeletion: true
@@ -13,9 +13,9 @@
     kind: MachineConfigPool
     name: master
     uid: 36a50017-aa22-11e9-9e86-02eb8e87567e
-  resourceVersion: "3475"
-  selfLink: /apis/machineconfiguration.openshift.io/v1/machineconfigs/rendered-master-7922d771acbd9d212c63b49ce17ff0b4
-  uid: 43946e13-aa22-11e9-9e86-02eb8e87567e
+  resourceVersion: "43399"
+  selfLink: /apis/machineconfiguration.openshift.io/v1/machineconfigs/rendered-master-f3613c0c705e7cd9696b267237be420f
+  uid: 285dbd8f-aa33-11e9-be58-02615f8d79ce
 spec:
   config:
     ignition:
@@ -41,7 +41,7 @@
         mode: 420
         path: /etc/etcd/etcd.conf
       - contents:
-          source: data:,apiVersion%3A%20v1%0Akind%3A%20Pod%0Ametadata%3A%0A%20%20name%3A%20etcd-member%0A%20%20namespace%3A%20openshift-etcd%0A%20%20labels%3A%0A%20%20%20%20k8s-app%3A%20etcd%0Aspec%3A%0A%20%20initContainers%3A%0A%20%20-%20name%3A%20discovery%0A%20%20%20%20image%3A%20%22registry.svc.ci.openshift.orA
+          source: data:,apiVersion%3A%20v1%0Akind%3A%20Pod%0Ametadata%3A%0A%20%20name%3A%20etcd-member%0A%20%20namespace%3A%20openshift-etcd%0A%20%20labels%3A%0A%20%20%20%20k8s-app%3A%20etcd%0Aspec%3A%0A%20%20initContainers%3A%0A%20%20-%20name%3A%20discovery%0A%20%20%20%20image%3A%20%22image-registry.openshift-imaA
           verification: {}
         filesystem: root
         mode: 420

which triggers a master rollout. Something to fix later.

mandre added a commit to shiftstack/shiftstack-ci that referenced this pull request Jul 24, 2019
@mandre
Fix build_mco.sh
9ac4fbf 
The image building script in MCO was renamed and changed to only
rebuild image based on the git index, meaning we have to commit changes
to git in order to test something.

I find this new behavior rather annoying in a dev workflow and changed
our script to use podman build/push instead.

See openshift/machine-config-operator#982 for
the upstream changes.
@mandre mandre mentioned this pull request Jul 24, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ashcrow ashcrow Awaiting requested review from ashcrow

1 more reviewer

@kikisdeliveryservice kikisdeliveryservice kikisdeliveryservice left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@runcom runcom

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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@cgwalters @LorbusChris @runcom @openshift-ci-robot @kikisdeliveryservice @openshift-merge-robot