Feat: Adding hostAliases in PodTemplate spec. #3889

xclud · 2021-04-17T19:26:24Z

Changes

Adding hostAliases in PodTemplate spec.

/kind feature

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

Feat: Add hostAliases in PodTemplate spec which provides Pod-level override of hostname resolution by modifing /etc/hosts.

tekton-robot · 2021-04-17T19:26:38Z

Hi @xclud. 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.

ghost

Thanks a lot for tackling this!

ghost · 2021-04-19T11:22:13Z

pkg/apis/pipeline/pod/template.go

+	// +optional
+	// +patchMergeKey=ip
+	// +patchStrategy=merge
+	HostAliases []corev1.HostAlias `json:"hostAliases,omitempty" patchStrategy:"merge" patchMergeKey:"ip" protobuf:"bytes,23,rep,name=hostAliases"`


I don't think strategic merge patch is supported on CRDs. We also don't have patchStrategy and patchMergeKey on any other fields in this struct. Suggest removing.

I made the changes and removed them. However, volumes field has patchStrategy and patchMergeKey, just to say.

Ah, you are right, thanks for pointing that out - I missed volumes when i looked!

ghost · 2021-04-19T11:31:00Z

/ok-to-test

xclud · 2021-04-19T12:59:13Z

/retest

ghost · 2021-04-19T13:29:31Z

@xclud you'll need to run ./hack/update-openapigen.sh I think to generate the open api changes. Also please squash commits down into 1 🙏

Re-running integration tests:

/test pull-tekton-pipeline-integration-tests

xclud · 2021-04-19T14:03:29Z

@sbwsg I donno how to squash commits in github (i am from gitlab background). Would you please help?

ghost · 2021-04-19T14:13:24Z

@sbwsg I donno how to squash commits in github (i am from gitlab background). Would you please help?

Sure, although I'm only from git-land so I only know the CLI command :D

Run git rebase -i HEAD^^^^
This will give you an editor window with the list of commits in your branch
Change all entries after the first in the list of commits so that they start with f . This means "fixup" which will result in all the commits after your first one being squashed together. It will also discard the commit message associated with each of the commits (except for your first one). If you want to keep the commit messages from those then use s instead of f .
Save and quit the editor

Now when you git show HEAD you should see that all your changes appear in a single commit. git log should now show just a single commit on top of whatever the branch had before.

ghost · 2021-04-19T14:14:23Z

Oh, almost forgot - after you do this and confirm that everything looks correct you'll need to force push to your remote branch with git push --force <your remote> <your branch>

ghost · 2021-04-19T14:23:46Z

The build tests are failing because ./hack/update-openapigen.sh has generated diffs that look like this:

-	Ref:     ref("github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.TaskResult"),
+	Ref:     ref("./pkg/apis/pipeline/v1beta1.TaskResult"),

I asked in Slack and it sounds like the reason this happens is because the script is running outside of GOPATH. We've got an issue open to tackle this here: #3884

@xclud can you try re-running ./hack/update-openapigen.sh from within your GOPATH to see if that correctly writes these Refs? One way to do this (according to that issue) is to symlink your Pipeline working directory to $GOPATH/src/github.com/tektoncd/pipeline 🤞

ghost · 2021-04-19T14:26:28Z

Oh no! I wrote update-codegen.sh when I should have written update-openapigen.sh Apologies! I'll update that previous comment.

linux-foundation-easycla · 2021-04-19T15:17:40Z

The committers are authorized under a signed CLA.

✅ Mahdi (3bb7ec5)

pkg/apis/pipeline/pod/template.go

pritidesai · 2021-04-19T20:39:21Z

Thank you @xclud for your contributions, changes look great 🙏

The build is failing with:

/home/prow/go/src/github.com/tektoncd/pipeline is out of date. Please run hack/update-codegen.sh

Please rerun the update-codegen.sh and hopefully it works for you.

@sbwsg awesome instructions on squashing the commits. 👍

xclud · 2021-04-19T20:48:02Z

Thank you @xclud for your contributions, changes look great 🙏

The build is failing with:
/home/prow/go/src/github.com/tektoncd/pipeline is out of date. Please run hack/update-codegen.sh
Please rerun the update-codegen.sh and hopefully it works for you.

@sbwsg awesome instructions on squashing the commits. 👍

I already ran update-codegen.sh. It only updates my go.sum file. Should i push it as well ?

ghost · 2021-04-19T21:00:09Z

I don't quite know why but the order of Dependencies in the generated openapi_generated.go file is changing. Notice below how pod.Template shifts position in the list.

 		Dependencies: []string{
-			"github.com/tektoncd/pipeline/pkg/apis/pipeline/pod.Template", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.Param", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.TaskRef", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.TaskRunResources", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.TaskSpec", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.WorkspaceBinding", "k8s.io/apimachinery/pkg/apis/meta/v1.Duration"},
+			"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.Param", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.TaskRef", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.TaskRunResources", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.TaskSpec", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.WorkspaceBinding", "github.com/tektoncd/pipeline/pkg/apis/pipeline/pod.Template", "k8s.io/apimachinery/pkg/apis/meta/v1.Duration"},
 	}

Very annoying! I wonder if this is related to go version? @xclud what version are you running?

I think the quickest way to get this resolved would be to hand-edit openapi_generated.go locally and move "github.com/tektoncd/pipeline/pkg/apis/pipeline/pod.Template" in those two lists to match the expectation of the test runner. Really sorry about that, it's not a great dev experience but hopefully it resolves the immediate problem.

pritidesai · 2021-04-20T18:39:53Z

Yay, the build is passing 🎉

There is an ongoing issue with integration test, not related to your changes, see @sbwsg's comment on another PR.

pritidesai · 2021-04-20T18:41:57Z

@xclud one last thing, please add some details in commit message, it's easier for folks later to understand the changes. 🙏

xclud · 2021-04-20T18:42:43Z

@sbwsg @pritidesai Thank you guys for your support. I appreciate it. I am from Windows background and git-clicks. I learned from you.

I squashed the commits.

xclud · 2021-04-20T18:53:36Z

@xclud one last thing, please add some details in commit message, it's easier for folks later to understand the changes. 🙏

I am messing up with git :D sorry guys. I try to fix it.

xclud · 2021-04-20T18:56:15Z

@sbwsg I tried to push a message-only commit and squash. Now i see all my changes are gone. I am stuck.

ghost · 2021-04-20T19:00:24Z

@xclud no worries; here are some git commands to rescue the changes:

Make sure you're on your branch
Run git reset --hard 00f3b3a
This should put you back in a good state with the code (back to here)
Run git commit --amend
This should show you a screen to edit the commit message.
Make edits and save.
git push --force <remote name> <branch name>

tekton-robot · 2021-04-20T19:03:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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:

~~OWNERS~~ [sbwsg]

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

xclud · 2021-04-20T19:05:12Z

@sbwsg Thanks again. I also updated the release notes.

ghost · 2021-04-20T19:06:09Z

@sbwsg Thanks again. I also updated the release notes.

Great work, thanks a lot for sticking with it through this whole process!

pritidesai · 2021-04-20T19:13:39Z

thanks @sbwsg for very helpful git pointers 🎉

Great work, thanks a lot for sticking with it through this whole process!

yup, 💯 thank you @xclud 👍 and welcome to the community 🙏

/lgtm

hopefully, the changes will be merged after we fix the integration tests 🤞

ghost · 2021-04-20T21:04:08Z

You'll need to rebase on top of main to get the integration tests passing.

git fetch upstream
git rebase upstream/main
git push --force <your remote> <branch name>

…of hostname resolution by modifing /etc/hosts.

xclud · 2021-04-21T06:37:02Z

@sbwsg Done. But it seems integration tests are still failing.

vdemeester · 2021-04-21T09:39:15Z

default 4m46s Normal VolumeDelete persistentvolume/pvc-03141f10-77ba-4fe2-9cfa-a29fcf067319 googleapi: Error 400: The disk resource 'projects/tekton-prow-9/zones/us-central1-b/disks/gke-tpipeline-e2e-cls1-pvc-03141f10-77ba-4fe2-9cfa-a29fcf067319' is already being used by 'projects/tekton-prow-9/zones/us-central1-b/instances/gke-tpipeline-e2e-cls138-default-pool-ed3cdb7a-hk83', resourceInUseByAnotherResource

Seems like too much rebuild happened at the same time 😓

/retest

xclud · 2021-04-21T19:39:53Z

/retest

ghost · 2021-04-21T19:41:11Z

/test pull-tekton-pipeline-integration-tests

pritidesai · 2021-04-21T19:58:58Z

/lgtm

xclud · 2021-04-21T20:31:27Z

Thank you guys. I am happy to see this PR is merged.

Can we also close #1750?

pritidesai · 2021-04-21T20:41:11Z

Can we also close #1750?

Yup, we can close #1750

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Apr 17, 2021

tekton-robot requested review from dibyom and vdemeester April 17, 2021 19:26

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 17, 2021

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 17, 2021

xclud mentioned this pull request Apr 17, 2021

No HostAliases provided? #1750

Closed

ghost reviewed Apr 19, 2021

View reviewed changes

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 Apr 19, 2021

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 19, 2021

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 19, 2021

ghost approved these changes Apr 19, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 19, 2021

pritidesai reviewed Apr 19, 2021

View reviewed changes

pkg/apis/pipeline/pod/template.go Outdated Show resolved Hide resolved

tekton-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 20, 2021

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 20, 2021

tekton-robot assigned pritidesai Apr 20, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 20, 2021

Add hostAliases in PodTemplate spec which provides Pod-level overrid …

e18bfed

…of hostname resolution by modifing /etc/hosts.

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 21, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 21, 2021

tekton-robot merged commit 1896d4e into tektoncd:main Apr 21, 2021

xclud deleted the feat-host-aliases branch April 21, 2021 20:31

tlawrie mentioned this pull request May 18, 2021

Tekton Client updated for 0.24 fabric8io/kubernetes-client#3140

Closed

Feat: Adding hostAliases in PodTemplate spec. #3889

Feat: Adding hostAliases in PodTemplate spec. #3889

Conversation

xclud commented Apr 17, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Apr 17, 2021

ghost left a comment

Choose a reason for hiding this comment

ghost Apr 19, 2021

Choose a reason for hiding this comment

xclud Apr 19, 2021

Choose a reason for hiding this comment

ghost Apr 19, 2021

Choose a reason for hiding this comment

ghost commented Apr 19, 2021

xclud commented Apr 19, 2021

ghost commented Apr 19, 2021

xclud commented Apr 19, 2021

ghost commented Apr 19, 2021

ghost commented Apr 19, 2021

ghost commented Apr 19, 2021 • edited by ghost Loading

ghost commented Apr 19, 2021

linux-foundation-easycla bot commented Apr 19, 2021 • edited Loading

pritidesai commented Apr 19, 2021

xclud commented Apr 19, 2021

ghost commented Apr 19, 2021

pritidesai commented Apr 20, 2021

pritidesai commented Apr 20, 2021

xclud commented Apr 20, 2021

xclud commented Apr 20, 2021

xclud commented Apr 20, 2021

ghost commented Apr 20, 2021

tekton-robot commented Apr 20, 2021

xclud commented Apr 20, 2021

ghost commented Apr 20, 2021

pritidesai commented Apr 20, 2021

ghost commented Apr 20, 2021

xclud commented Apr 21, 2021

vdemeester commented Apr 21, 2021

xclud commented Apr 21, 2021

ghost commented Apr 21, 2021

pritidesai commented Apr 21, 2021

xclud commented Apr 21, 2021

pritidesai commented Apr 21, 2021

xclud commented Apr 17, 2021 •

edited

Loading

ghost commented Apr 19, 2021 •

edited by ghost

Loading

linux-foundation-easycla bot commented Apr 19, 2021 •

edited

Loading