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

Feat: Adding hostAliases in PodTemplate spec. #3889

Merged
merged 1 commit into from
Apr 21, 2021
Merged

Feat: Adding hostAliases in PodTemplate spec. #3889

merged 1 commit into from
Apr 21, 2021

Conversation

xclud
Copy link
Contributor

@xclud xclud commented Apr 17, 2021

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 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 tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 17, 2021
@tekton-robot
Copy link
Collaborator

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.

@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 Apr 17, 2021
@xclud xclud mentioned this pull request Apr 17, 2021
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Thanks a lot for tackling this!

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

@ghost
Copy link

ghost commented Apr 19, 2021

/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 Apr 19, 2021
@xclud
Copy link
Contributor Author

xclud commented Apr 19, 2021

/retest

@ghost
Copy link

ghost commented Apr 19, 2021

@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

@tekton-robot 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
@xclud
Copy link
Contributor Author

xclud commented Apr 19, 2021

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

@ghost
Copy link

ghost commented Apr 19, 2021

@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

  1. Run git rebase -i HEAD^^^^
  2. This will give you an editor window with the list of commits in your branch
  3. 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 .
  4. 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
Copy link

ghost commented Apr 19, 2021

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
Copy link

ghost commented Apr 19, 2021

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
Copy link

ghost commented Apr 19, 2021

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
Copy link

linux-foundation-easycla bot commented Apr 19, 2021

CLA Signed

The committers are authorized under a signed CLA.

@tekton-robot 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
@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 19, 2021
@pritidesai
Copy link
Member

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
Copy link
Contributor Author

xclud commented Apr 19, 2021

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
Copy link

ghost commented Apr 19, 2021

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
Copy link
Member

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
Copy link
Member

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

@xclud
Copy link
Contributor Author

xclud commented Apr 20, 2021

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

@tekton-robot 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
@xclud
Copy link
Contributor Author

xclud commented Apr 20, 2021

@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
Copy link
Contributor Author

xclud commented Apr 20, 2021

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

@ghost
Copy link

ghost commented Apr 20, 2021

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

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

@tekton-robot
Copy link
Collaborator

[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:

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 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
@xclud
Copy link
Contributor Author

xclud commented Apr 20, 2021

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

@ghost
Copy link

ghost commented Apr 20, 2021

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

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

@pritidesai
Copy link
Member

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 🤞

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

ghost commented Apr 20, 2021

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

  1. git fetch upstream
  2. git rebase upstream/main
  3. git push --force <your remote> <branch name>

…of hostname resolution by modifing /etc/hosts.
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 21, 2021
@xclud
Copy link
Contributor Author

xclud commented Apr 21, 2021

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

@vdemeester
Copy link
Member

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
Copy link
Contributor Author

xclud commented Apr 21, 2021

/retest

@ghost
Copy link

ghost commented Apr 21, 2021

/test pull-tekton-pipeline-integration-tests

@pritidesai
Copy link
Member

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 21, 2021
@tekton-robot tekton-robot merged commit 1896d4e into tektoncd:main Apr 21, 2021
@xclud
Copy link
Contributor Author

xclud commented Apr 21, 2021

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

Can we also close #1750?

@xclud xclud deleted the feat-host-aliases branch April 21, 2021 20:31
@pritidesai
Copy link
Member

Can we also close #1750?

Yup, we can close #1750

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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants