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

Replace dump-init with go-init #2130

Merged
merged 3 commits into from
Sep 19, 2019
Merged

Replace dump-init with go-init #2130

merged 3 commits into from
Sep 19, 2019

Conversation

mohammedzee1000
Copy link
Contributor

@mohammedzee1000 mohammedzee1000 commented Sep 16, 2019 •
edited

What is the purpose of this change? What does it change?

This PR makes use of go-init instead of dumb-init to avoid having to maintain our own rpm for dumb-init

How to test changes?

All standard workflows should pass

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. size/S labels Sep 16, 2019
@kadel
Copy link
Member

kadel commented Sep 16, 2019

for reference: changes on odo-init-image - redhat-developer/odo-init-image#35

kadel
kadel reviewed Sep 16, 2019
View reviewed changes
pkg/occlient/templates.go Outdated
@@ -83,15 +83,13 @@ func generateSupervisordDeploymentConfig(commonObjectMeta metav1.ObjectMeta, com
Ports: commonImageMeta.Ports,
// Run the actual supervisord binary that has been mounted into the container
Command: []string{
"/opt/odo/bin/dumb-init",
"--",
"/bin/bash",
Copy link
Member

Choose a reason for hiding this comment

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

we probably don't need bash here. Executing just /opt/odo/bin/go-init should be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@mohammedzee1000 mohammedzee1000 changed the title WIP Go init Replace dump-init with go-init Sep 17, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Sep 17, 2019
kadel
kadel reviewed Sep 17, 2019
View reviewed changes
pkg/occlient/occlient.go Outdated
@@ -95,7 +95,7 @@ const (

// Default Image that will be used containing the supervisord binary and assembly scripts
// use getBoostrapperImage() function instead of this variable
defaultBootstrapperImage = "quay.io/openshiftdo/init:0.10.0"
defaultBootstrapperImage = "quay.io/mohammedzee1000/odo-init-image:go-init"
Copy link
Member

@kadel kadel Sep 17, 2019 •
edited

Choose a reason for hiding this comment

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

Just so we don' forget. This needs to point to quay.io/openshiftdo/init:0.11.0 once the redhat-developer/odo-init-image#35 and new version of the image is released.

Only after that, we can merge this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that is why I specifically marked that change in a separate commit, so that it can be dropped :)

Copy link
Member

Choose a reason for hiding this comment

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

quay.io/openshiftdo/init:0.11.0 is built. @mohammedzee1000 please update it to this image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@kadel kadel mentioned this pull request Sep 17, 2019
Merged
@kadel
Copy link
Member

kadel commented Sep 18, 2019

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Sep 18, 2019
@kadel
Copy link
Member

kadel commented Sep 18, 2019 

[odo] error: build error: After retrying 2 times, Pull image still failed due to error: while pulling "docker://image-registry.openshift-image-registry.svc:5000/openshift/php@sha256:64a0c4fb363357761a9f5550eecda1476d7f737e5f1d0863919d1f60985811ba" as "image-registry.openshift-image-registry.svc:5000/openshift/php@sha256:64a0c4fb363357761a9f5550eecda1476d7f737e5f1d0863919d1f60985811ba": Error initializing source docker://image-registry.openshift-image-registry.svc:5000/openshift/php@sha256:64a0c4fb363357761a9f5550eecda1476d7f737e5f1d0863919d1f60985811ba: pinging docker registry returned: Get https://image-registry.openshift-image-registry.svc:5000/v2/: dial tcp 172.30.102.228:5000: connect: no route to host

/retest

@mohammedzee1000
Copy link
Contributor Author

s2i related error

[odo] subprocess exited with status 255
[odo] subprocess exited with status 255
[odo] error: build error: error building at STEP "RUN /usr/libexec/s2i/assemble": exit status 255

/retest

@girishramnani
Copy link
Contributor

looks good to me
/lgtm

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

/retest

@openshift-merge-robot openshift-merge-robot merged commit d151883 into redhat-developer:master Sep 19, 2019
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Sep 19, 2019 •
edited

@mohammedzee1000: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/v4.2-benchmark 419557f link /test v4.2-benchmark
ci/prow/v4.2-e2e-scenarios 419557f link /test v4.2-e2e-scenarios

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@dharmit dharmit mentioned this pull request Sep 26, 2019
Merged
@rm3l rm3l added the estimated-size/S (5-10) Rough sizing for Epics. Less then one sprint of work for one person label Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kadel kadel kadel left review comments

@cdrage cdrage Awaiting requested review from cdrage

Assignees

@girishramnani girishramnani

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. estimated-size/S (5-10) Rough sizing for Epics. Less then one sprint of work for one person lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mohammedzee1000 @kadel @openshift-ci-robot @girishramnani @rm3l @openshift-merge-robot