Skip to content

Remove the redundant resources in channel-broker and mt-channel-broker#3032

Merged
knative-prow-robot merged 4 commits into
knative:masterfrom
houshengbo:remove-repeated-rsources
May 2, 2020
Merged

Remove the redundant resources in channel-broker and mt-channel-broker#3032
knative-prow-robot merged 4 commits into
knative:masterfrom
houshengbo:remove-repeated-rsources

Conversation

@houshengbo
Copy link
Copy Markdown

@houshengbo houshengbo commented Apr 21, 2020 •
edited
Loading

Fixes #3033

Proposed Changes

  • This PR removed the recursive way (-R) to look up all the yaml files for channel-broker and mt-channel-broker.
  • Based on the current file structure, only eventing-core needs to be built recursively with all the files under config/core.

Release Note

Docs

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 21, 2020
@knative-prow-robot knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. area/test-and-release Test infrastructure, tests or release labels Apr 21, 2020
matzew
matzew reviewed Apr 22, 2020
View reviewed changes
Comment thread hack/release.sh Outdated
["mt-channel-broker.yaml"]="config/brokers/mt-channel-broker"
["in-memory-channel.yaml"]="config/channels/in-memory-channel"
["upgrade-to-v0.14.0.yaml"]="config/upgrade/v0.14.0"
["eventing-core.yaml"]="-R -f config/core"
Copy link
Copy Markdown
Member

@matzew matzew Apr 22, 2020

Choose a reason for hiding this comment

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

I do not like that declaration here. I liked it before better

how about doing some more explicit ko resolve runs, like serving:

https://github.com/knative/serving/blob/master/hack/generate-yamls.sh#L86-L104

Copy link
Copy Markdown
Member

@matzew matzew Apr 22, 2020

Choose a reason for hiding this comment

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

/hold

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 22, 2020
@knative-prow-robot knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 22, 2020
vaikas
vaikas reviewed Apr 24, 2020
View reviewed changes
Comment thread hack/release.sh Outdated
TAG="latest"
fi

readonly UPGRADE_JOB="upgrade-to-${TAG}.yaml"
Copy link
Copy Markdown
Contributor

@vaikas vaikas Apr 24, 2020

Choose a reason for hiding this comment

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

Do you also need to update the config/upgrade/v0.14/README.md to point to the newly renamed file?

Copy link
Copy Markdown
Member

@matzew matzew Apr 27, 2020

Choose a reason for hiding this comment

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

and than I think I find it strange that the 0.14 than would have a TAG that is different than 0.14 (e.g. latest or 0.15)

Copy link
Copy Markdown
Author

@houshengbo houshengbo Apr 27, 2020

Choose a reason for hiding this comment

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

I will keep the folder of config/upgrade/v0.14.0 unchanged. If we need script for upgrade in future version, we can change the release.sh later.

aliok
aliok reviewed Apr 24, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@aliok aliok left a comment

Choose a reason for hiding this comment

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

Can't we come up with a solution that does some post-processing to remove duplicate resources?
Or something that "remembers" if a file is added to an artifact or not?
IMO that would be more stable (as we won't have the problem of some artifacts have -R and some don't) and safe.

@houshengbo
Copy link
Copy Markdown
Author

houshengbo commented Apr 27, 2020

@aliok If we know how to pick up the content for the released yamls, why do we blindly consolidate everything, and then post-process? Why not just orchestrate the correct way at the beginning?

@matzew
Copy link
Copy Markdown
Member

matzew commented Apr 29, 2020

/lgtm

/assign @vaikas for final approverz

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2020
@matzew
Copy link
Copy Markdown
Member

matzew commented May 1, 2020

@vaikas any comment?

@vaikas
Copy link
Copy Markdown
Contributor

vaikas commented May 1, 2020

I'm so sorry for the hold up :( my bad
/lgtm
/approve

@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented May 1, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: houshengbo, vaikas

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:

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 1, 2020
@houshengbo
Copy link
Copy Markdown
Author

houshengbo commented May 2, 2020

/hold cancel

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 2, 2020
@knative-prow-robot knative-prow-robot merged commit 2696a18 into knative:master May 2, 2020
@grantr grantr mentioned this pull request May 11, 2020
Closed
@aliok aliok mentioned this pull request Sep 25, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matzew matzew matzew left review comments

@aliok aliok aliok left review comments

@steuhs steuhs Awaiting requested review from steuhs

@yt3liu yt3liu Awaiting requested review from yt3liu

+1 more reviewer

@vaikas vaikas vaikas left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@matzew matzew

@vaikas vaikas

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release Test infrastructure, tests or release cla: yes Indicates the PR's author has signed the CLA. 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.

All resources in channel-broker and mt-channel-broker have been defined twice

6 participants

@houshengbo @matzew @vaikas @knative-prow-robot @aliok @googlebot