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

Allow for setting imagePullSecrets #2547

Merged
merged 1 commit into from
Jun 5, 2020

Conversation

NikeNano
Copy link

@NikeNano NikeNano commented May 5, 2020

Changes

NOTE: I am not assigned to the issue, but fixed it since I saw that it was stale for some time and thought it would be fun to try to fix it.

This PR updates the PodTemplate with ImagePullSecret to allow for setting image pull secrets. This PR aims to fix issue #1779.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Allow to specify ImagePullSecrets in the PodTemplate.

@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 5, 2020
@tekton-robot
Copy link
Collaborator

Hi @NikeNano. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@tekton-robot tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 5, 2020
@NikeNano
Copy link
Author

NikeNano commented May 5, 2020

I have some issues with git and cant squash the commit, will fix it and squash according to the best practice.

pkg/pod/pod_test.go Outdated Show resolved Hide resolved
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

Thank you for this. I think it's a handy feature to have.
@imjasonh @skaegi
/approve

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2020
pkg/pod/pod_test.go Outdated Show resolved Hide resolved
@afrittoli
Copy link
Member

/ok-to-test

@tekton-robot tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 7, 2020
@dibyom
Copy link
Member

dibyom commented May 11, 2020

The feature seems fine to me. Wondering if there are any implications for setting the imagePullSecret here vs the service account as mentioned in https://github.com/tektoncd/pipeline/blob/master/docs/container-contract.md#entrypoint

Perhaps @imjasonh knows ?

@afrittoli
Copy link
Member

The feature seems fine to me. Wondering if there are any implications for setting the imagePullSecret here vs the service account as mentioned in https://github.com/tektoncd/pipeline/blob/master/docs/container-contract.md#entrypoint

Perhaps @imjasonh knows ?

Good point

@NikeNano
Copy link
Author

friendly ping @imjasonh :)

@vdemeester
Copy link
Member

/kind feature

@tekton-robot tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label May 19, 2020
@imjasonh
Copy link
Member

AIUI this will make image pull secrets available, alongside any secrets available to the specified SA. So this shouldn't break any existing users, and should give more options to power users.

@NikeNano
Copy link
Author

NikeNano commented May 20, 2020

@dibyom is there anything more I can do, except fixing the merge conflict? I have not squashed the commits since there is a merge in the middle and where not able to fix it :(.

@vdemeester
Copy link
Member

vdemeester commented May 20, 2020

@NikeNano you may need to get the changes and re-apply them on a clean state, something like the following could work

# Making sure we are on the branch :)
git checkout image_pull_request
# Merge and fix conflict
git merge origin/master
# … fix conflict
git merge --continue
# generate a diff
git diff origin/master..HEAD > mypr.diff
# backup the branch
git checkout -b backupbranch
# force-reset this PR branch
git checkout origin/master -B image_pull_secret
# apply the diff
git apply mypr.diff
# commit
git commit -s 
# force push
git push --force-with-lease origin image_pull_secret
# clean mypr.diff
rm mypr.diff

This should be roughly the equivalent of stashing the changes into one commit.

In the future, when working on OSS project, it tends to be better to use git rebase to update the branches which are used in a pull-request 😉

/hold

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 20, 2020
@NikeNano
Copy link
Author

@NikeNano you may need to get the changes and re-apply them on a clean state, something like the following could work

# Making sure we are on the branch :)
git checkout image_pull_request
# Merge and fix conflict
git merge origin/master
# … fix conflict
git merge --continue
# generate a diff
git diff origin/master..HEAD > mypr.diff
# backup the branch
git checkout -b backupbranch
# force-reset this PR branch
git checkout origin/master -B image_pull_secret
# apply the diff
git apply mypr.diff
# commit
git commit -s 
# force push
git push --force-with-lease origin image_pull_secret
# clean mypr.diff
rm mypr.diff

This should be roughly the equivalent of stashing the changes into one commit.

In the future, when working on OSS project, it tends to be better to use git rebase to update the branche which are used in a pull-request 😉

/hold

Thanks @vdemeester will fix this tomorrow!

@NikeNano NikeNano force-pushed the image_pull_secret branch 3 times, most recently from 92faa0f to 9e882ae Compare May 21, 2020 20:01
@NikeNano
Copy link
Author

@vdemeester I have squashed the commit messages.

Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

Something went wrong in the merge of the docs.
Once this is fixed we can lift the hold and get this merged :)

docs/podtemplates.md Outdated Show resolved Hide resolved
In K8s, Pods can specify a imagePullSecrets which identifies K8s secrets that the container runtime should use to authorize container image pulls when starting a Pod. To achive this imageSecrets is added to PodTemplate.

Signed-off-by: NikeNano <[email protected]>
Co-authored-by: JohanWork <[email protected]>
@NikeNano
Copy link
Author

@afrittoli should be fixed now!

@NikeNano
Copy link
Author

Is there something I can fix @afrittoli :)

@NikeNano
Copy link
Author

NikeNano commented Jun 1, 2020

any updates @afrittoli @vdemeester :)

@vdemeester vdemeester removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 2, 2020
@afrittoli
Copy link
Member

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 4, 2020
@vdemeester
Copy link
Member

/test pull-tekton-pipeline-integration-tests

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. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants