Skip to content

templates: Add a templates/common/, move pull secret there#627

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
cgwalters:template-common
May 16, 2019
Merged

templates: Add a templates/common/, move pull secret there#627
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
cgwalters:template-common

Conversation

@cgwalters
Copy link
Copy Markdown
Member

@cgwalters cgwalters commented Apr 11, 2019 •
edited
Loading

A lot of templates are duplicated between master/worker. There's already
a _base for things not specific to a platform, so introduce _base/_base
which is templates that are shared between master/worker.

Start by moving the pull secret there. If this PR is approved I'll
do all of the common files.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 11, 2019
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 11, 2019
@cgwalters
Copy link
Copy Markdown
Member Author

cgwalters commented Apr 11, 2019

This was bugging me for a while...doesn't quite pass tests but let me know if you guys like the approach.

@kikisdeliveryservice
Copy link
Copy Markdown
Contributor

kikisdeliveryservice commented Apr 11, 2019 •
edited
Loading

@cgwalters is the idea to take the set of worker files that also exist (and are identical) in master dir and move to base?

generally, (without looking at the code that much) i think i like the idea since it allows us to be explicit & consistent with the files that are exactly the same between the two.

as a note: you do have some file name issues currently, a few end up being templates/_base/_base...

@runcom
Copy link
Copy Markdown
Member

runcom commented Apr 11, 2019

This was bugging me for a while...doesn't quite pass tests but let me know if you guys like the approach.

I like it

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 13, 2019
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 14, 2019
@cgwalters
Copy link
Copy Markdown
Member Author

cgwalters commented May 14, 2019

Rebased 🏄‍♂️ - this depends on #749 too now.

Keeping as WIP for feedback.

@cgwalters cgwalters changed the title WIP: change master/worker to have a common _base WIP: templates: Add a _base/_base, move pull secret there May 14, 2019
@runcom
Copy link
Copy Markdown
Member

runcom commented May 15, 2019

needs a rebase to pick up #749

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 15, 2019
@cgwalters
Copy link
Copy Markdown
Member Author

cgwalters commented May 15, 2019

🏄‍♂️ rebased!

@cgwalters cgwalters changed the title WIP: templates: Add a _base/_base, move pull secret there templates: Add a _base/_base, move pull secret there May 15, 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. label May 15, 2019
kikisdeliveryservice
kikisdeliveryservice previously requested changes May 15, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@kikisdeliveryservice kikisdeliveryservice left a comment •
edited
Loading

Choose a reason for hiding this comment

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

we end up with a name: templates/_base/_base/files/pull-secret.yaml I think the double _base in the name seems a little confusing no? Why not _base/shared or shared/_base something clearer?

@DanyC97
Copy link
Copy Markdown
Contributor

DanyC97 commented May 15, 2019 •
edited
Loading

@cgwalters if you don't mind adding my thought, i think (maybe is a preference thing) would be a bit cleaner if we had /templates/_base/{common|shared}/ (either of the 2 words is fine) instead of templates/_base/_base.

IMHO it much more natural thinking what it is w/o questioning the diff between _base and _base/_base

@cgwalters
templates: Add a templates/common/, move pull secret there
19c828b 
A lot of templates are duplicated between master/worker.  Create a `templates/common/`
which will contain things shared between the two.

Start by moving the pull secret there.  If this PR is approved I'll
do all of the common files.
@cgwalters
Copy link
Copy Markdown
Member Author

cgwalters commented May 15, 2019

Updated to use templates/common/

@LorbusChris
Copy link
Copy Markdown
Contributor

LorbusChris commented May 16, 2019

/retest

@kikisdeliveryservice kikisdeliveryservice dismissed their stale review May 16, 2019 16:53

changes made as requested

@kikisdeliveryservice
Copy link
Copy Markdown
Contributor

kikisdeliveryservice commented May 16, 2019

thanks for updating @cgwalters !!!

/LGTM

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

openshift-ci-robot commented May 16, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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,kikisdeliveryservice]

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

@kikisdeliveryservice kikisdeliveryservice changed the title templates: Add a _base/_base, move pull secret there templates: Add a templates/common/, move pull secret there May 16, 2019
@openshift-merge-robot openshift-merge-robot merged commit 6aea199 into openshift:master May 16, 2019
@cgwalters cgwalters mentioned this pull request May 20, 2019
Merged
cgwalters added a commit to cgwalters/machine-config-operator that referenced this pull request May 21, 2019
@cgwalters
templates: Move more files into common/
faa5a9c 
Follow up to openshift#627
which only moved the pull secret to avoid conflicting with other PRs,
but since that went well let's do the rest of the `_base/files` dir.

Via this script:
```
set -euo pipefail
master=templates/master/00-master
worker=templates/worker/00-worker
(cd ${master} && find _base/files -type f) | while read f; do
  if [ -f "${worker}/${f}" ] && cmp -s ${master}/${f} ${worker}/${f}; then
    git mv ${master}/$f templates/common/_base/files
    git rm ${worker}/$f
  fi
done
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@LorbusChris LorbusChris LorbusChris approved these changes

@kikisdeliveryservice kikisdeliveryservice kikisdeliveryservice left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@kikisdeliveryservice kikisdeliveryservice

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

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