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

add -thin-manifest-dir flag #141

Merged
merged 5 commits into from Oct 11, 2019
Merged

add -thin-manifest-dir flag #141

merged 5 commits into from Oct 11, 2019

Conversation

@listx
Copy link
Contributor

listx commented Oct 11, 2019

Thin manifests are like regular manifests but instead of having images: ... defined directly, they have a imagesPath: ... field that links to an images YAML that resides in a separate file.

I could have added a corresponding -thin-manifest flag, but decided not to because this is the minimal change necessary for the actual use case in k8s.io repo.

Fixes #138

/hold
/assign @justinsb

@listx

This comment has been minimized.

Copy link
Contributor Author

listx commented Oct 11, 2019

I plan to add an e2e test for this flag later today; let's hold off until we can get that working as well.

listx added 3 commits Oct 10, 2019
This forces users to have only 1 promoter manifest, named
"promoter-manifest.yaml" defined per directory if using the
-manifest-dir flag.

This makes using the -manifest-dir flag more secure for PRs from the
community because we will no longer allow any *.yaml to be read in.
This is just like -manifest-dir, but instead of expecting promoter
manifests that have both

    registries: ...

and

    images: ...

information, it only expects so-called thin manifests that look like

    registries: ...
    imagesPath: <path to images.yaml>

. These thin manifests are designed to be owned by a select few, and
they will refer to images.yaml files that reside in other folder(s) with
less restrictive ACLs. The point is to force PRs to only change the
images.yaml portions of manifests, and *NEVER* the 'registries:' part,
which contains important metadata that should rarely change.
@listx listx force-pushed the listx:master branch from 3104e1a to bb4a45d Oct 11, 2019
@listx listx force-pushed the listx:master branch from bb4a45d to f92b042 Oct 11, 2019
@listx listx mentioned this pull request Oct 11, 2019
1 of 4 tasks complete

// Skip non-YAML files.
if !strings.HasSuffix(path, ".yaml") {
// is that the file must be named "promoter-manifest.yaml".

This comment has been minimized.

Copy link
@justinsb

justinsb Oct 11, 2019

Contributor

Very Henry Ford of you to describe this as the "only" restriction ;-)

// Skip non-YAML files.
if !strings.HasSuffix(path, ".yaml") {
// is that the file must be named "promoter-manifest.yaml".
if !strings.HasSuffix(path, "/promoter-manifest.yaml") {

This comment has been minimized.

Copy link
@justinsb

justinsb Oct 11, 2019

Contributor

Checking info.Name() would avoid problems on Windows or if we didn't have a parent dir

@justinsb

This comment has been minimized.

Copy link
Contributor

justinsb commented Oct 11, 2019

LGTM - two very minor nits that you can fix separately if you agree / think it's worth it.

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 11, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 11, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, listx

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

@listx

This comment has been minimized.

Copy link
Contributor Author

listx commented Oct 11, 2019

Thanks Justin, will address those nits in a new PR!

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit a86d619 into kubernetes-sigs:master Oct 11, 2019
4 of 5 checks passed
4 of 5 checks passed
tide Not mergeable. Should not have do-not-merge/hold label.
Details
cla/linuxfoundation listx authorized
Details
pull-cip-e2e Job succeeded.
Details
pull-cip-lint Job succeeded.
Details
pull-cip-unit-tests Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.