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.

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

Enable bindata #229

Merged
merged 5 commits into from Dec 18, 2019
Merged

Conversation

sanchezl
Copy link
Contributor

@sanchezl sanchezl commented Dec 11, 2019

Use bindata (instead of inline code) to create the oauth-openshift deployment.

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 11, 2019
Copy link
Member

@stlaz stlaz left a comment

Choose a reason for hiding this comment

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

please mark the highlighted volumes as non-optional and move the auto-generated content to a separate commit, otherwise LGTM!

bindata/oauth-openshift/deployment.yaml Outdated Show resolved Hide resolved
bindata/oauth-openshift/deployment.yaml Outdated Show resolved Hide resolved
bindata/oauth-openshift/deployment.yaml Outdated Show resolved Hide resolved
bindata/oauth-openshift/deployment.yaml Outdated Show resolved Hide resolved
bindata/oauth-openshift/deployment.yaml Outdated Show resolved Hide resolved
bindata/oauth-openshift/deployment.yaml Outdated Show resolved Hide resolved
Comment on lines 53 to 55
if [ -s
/var/config/system/configmaps/v4-0-config-system-trusted-ca-bundle/ca-bundle.crt
]; then
Copy link
Member

Choose a reason for hiding this comment

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

This condition might be easier to read if it's just one line

fi
exec oauth-server osinserver
--config=/var/config/system/configmaps/v4-0-config-system-cliconfig/v4-0-config-system-cliconfig
--v=${LOG_LEVEL}
Copy link
Member

Choose a reason for hiding this comment

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

These line get squashed in the real deployment so that the args look like this:

      if [ -s /var/config/system/configmaps/v4-0-config-system-trusted-ca-bundle/ca-bundle.crt ]; then
          echo "Copying system trust bundle"
          cp -f /var/config/system/configmaps/v4-0-config-system-trusted-ca-bundle/ca-bundle.crt /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem
      fi exec oauth-server osinserver --config=/var/config/system/configmaps/v4-0-config-system-cliconfig/v4-0-config-system-cliconfig --v=2

As @soltysh pointed out earlier in his yaml-multiline findings, you may want to start with

args:
  - |

which is supposed to keep the newlines.

See https://yaml-multiline.info/ which is quite helpful

You'll have to add backslashes before each line ending in the exec part I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stlaz what do you think of running the "copy system bundle" part in an init container? Then the cmd/args for this container will be more maintainable?

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't you need a volume for such a change to persist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think an EmptyDir volume would do as this is the only file expected in the target directory. But I'll leave that for another day since this is working as is.

@sanchezl sanchezl force-pushed the enable-bindata branch 3 times, most recently from 555198d to 93537e2 Compare December 16, 2019 15:12
@sanchezl
Copy link
Contributor Author

/retest

@sanchezl
Copy link
Contributor Author

/test e2e-aws

- name: v4-0-config-system-session
secret:
secretName: v4-0-config-system-session
defaultMode: 420
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks suspicious/wrong. Aren't these supposed to be octal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

420 decimal == 0644 octal, which is the default anyway, so I have removed these lines.

@deads2k
Copy link
Contributor

deads2k commented Dec 17, 2019

I'm glad to see this kind of a change

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 17, 2019
@sanchezl
Copy link
Contributor Author

/test e2e-aws

2 similar comments
@sanchezl
Copy link
Contributor Author

/test e2e-aws

@sanchezl
Copy link
Contributor Author

/test e2e-aws

@mfojtik
Copy link
Member

mfojtik commented Dec 18, 2019

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, mfojtik, sanchezl

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-merge-robot openshift-merge-robot merged commit b3bd2c5 into openshift:master Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants