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

use secrets in sample templates #12757

Merged
merged 1 commit into from
Feb 3, 2017

Conversation

bparees
Copy link
Contributor

@bparees bparees commented Feb 1, 2017

No description provided.

@bparees
Copy link
Contributor Author

bparees commented Feb 1, 2017

@jim-minter ptal

@bparees
Copy link
Contributor Author

bparees commented Feb 1, 2017

[testextended][extended:core(builds)]

@bparees
Copy link
Contributor Author

bparees commented Feb 1, 2017

[test]

@bparees bparees force-pushed the template_secrets branch 5 times, most recently from 2f60301 to 82c3e8d Compare February 2, 2017 03:36
@smarterclayton
Copy link
Contributor

Previous run failed with a GCE error which I have fixed.

@jim-minter
Copy link
Contributor

That's already more churn than I guess was anticipated, and following the subsequent comments imply more of it still - perhaps worth ignoring them? :-/

  • The removal of the ADMIN_* template parameters implies making updates to README.md, but this may well have quite a lot of bit rot anyway and it might be worth just opening a ticket to rescan through it properly at some other time.

  • Was there a recent effort to move from json->yaml for these kinds of file?

  • Personally I'm in two minds about specifying all these no-op hooks ("/bin/true"), but they do have the advantage their existence brought to the surface the fact that the DC probably isn't labelling these properly at the moment.

@bparees
Copy link
Contributor Author

bparees commented Feb 2, 2017

That's already more churn than I guess was anticipated

yeah, more than i anticipated too :(

The removal of the ADMIN_* template parameters implies making updates to README.md, but this may well have quite a lot of bit rot anyway and it might be worth just opening a ticket to rescan through it properly at some other time.

yeah i was just removing them because they are unused (weirdly we weren't even passing them to the DB pod, we were only passing them to the app pod, not sure what the history is there. maybe once upon a time the app pod was using admin creds to setup the DB or something). I can fix the readme.

Was there a recent effort to move from json->yaml for these kinds of file?

there was, but @PI-Victor was doing it and i think ultimately it was probably a mistake to try to do them all in a single PR, I think we need to go back and do chunks at a time that can be more easily tested and reviewed (eg do all the extended test fixtures in one PR).

In any case i'm not adding that churn to this PR :)

Personally I'm in two minds about specifying all these no-op hooks ("/bin/true"), but they do have the advantage their existence brought to the surface the fact that the DC probably isn't labelling these properly at the moment.

unfortunately this template really serves two purposes:

  1. as an example of how to do certain things in templates (it's probably no longer serving that purpose...this was the first template that ever existed and the only one, for a while)
  2. it's leveraged by various tests meaning they have certain expectations that the template will exercise certain functionality

so stripping out "no-op" stuff will potentially remove test coverage.

@bparees
Copy link
Contributor Author

bparees commented Feb 2, 2017

flake #11887

@bparees
Copy link
Contributor Author

bparees commented Feb 2, 2017

@jim-minter readme updated.

@openshift-bot
Copy link
Contributor

Evaluated for origin testextended up to 24cbd1f

@PI-Victor
Copy link
Contributor

need to make a large rebase of that PR (not gonna lie, not looking fw to that), i still think it can pass if it's of immediate interest.

@PI-Victor
Copy link
Contributor

also, i haven't touched the extended tests fixtures. it only contains the examples folder. with readmes and related code.

@jim-minter
Copy link
Contributor

@bparees: lgtm

@bparees
Copy link
Contributor Author

bparees commented Feb 2, 2017

flake #12773
[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 24cbd1f

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/1061/) (Base Commit: 00fbbdd) (Extended Tests: core(builds))

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 24cbd1f

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13531/) (Base Commit: 89ed457)

@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 3, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13545/) (Base Commit: 4d6d4ed) (Image: devenv-rhel7_5841)

@openshift-bot openshift-bot merged commit f6d6014 into openshift:master Feb 3, 2017
@bparees bparees deleted the template_secrets branch February 7, 2017 19:14
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.

5 participants