-
Notifications
You must be signed in to change notification settings - Fork 590
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
Remove legacy code #532
Remove legacy code #532
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: scothis 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 |
/hold I wanted to see what the repo would look like if we removed all legacy code. I'm not sure if it is a good idea to merge this PR or not. |
/assign @vaikas-google I want to allow plenty of time for people to yell if they think we should persevere the old and new worlds side-by-side. I'm currently leaning towards a clean slate. As a bonus, it should help our coverage stats. |
we need to keep all of this around until the samples can be rewritten in knative/docs |
+1 to @n3wscott
I'd rather remove this once we have at least one working new example,
assuming that our current docs are pointing at our HEAD releases, rather
than a release branch that no one remembers creating. :-D
…On Mon, Oct 22, 2018 at 9:10 AM Scott Nichols ***@***.***> wrote:
we need to keep all of this around until the samples can be rewritten in
knative/docs
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#532 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AHlyN_IL3wzKi64AnJWOl6zhuge3JfVbks5une4LgaJpZM4XxPfA>
.
--
Evan Anderson <argent@google.com>
|
Each of the samples in knative/docs uses a source, which has moved to knative/eventing-sources. I can submit a PR that removes all of the eventing samples since they will each need to be significantly updated to the point where it's essentially a new sample. Another option is to cut a 0.1 release with what's in the repo today, pin the current samples against that and then focus all of our effort on 0.2. There would be zero backwards compatibility between 0.1 and 0.2. |
If we've moved the sources to eventing-sources and are including the
current Sources there, then I think we can delete these after the following
steps:
1. Make sure that we are generating release builds & yaml (nightly) for
eventing-sources.
2. Update the docs to point to the releases from point 1
3. Submit this PR.
I know that we've been steering people away from using eventing to some
extent, but I'd like to have the docs still be in a working shape for the
old setup until we have the new Channel & Provisioner working for in-memory
at least (which should be fairly soon, I think).
…On Tue, Oct 23, 2018 at 9:10 AM Scott Andrews ***@***.***> wrote:
Each of the samples in knative/docs uses a source, which has moved to
knative/eventing-sources. I can submit a PR that removes all of the
eventing samples since they will each need to be significantly updated to
the point where it's essentially a new sample.
Another option is to cut a 0.1 release with what's in the repo today, pin
the current samples against that and then focus all of our effort on 0.2.
There would be zero backwards compatibility between 0.1 and 0.2.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#532 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AHlyNylKj7wELruBqIwrJJ-ukVjzL2xoks5unz-HgaJpZM4XxPfA>
.
--
Evan Anderson <argent@google.com>
|
+1 I'd like to get to a state where our existing samples (or an agreed upon subset of them) actually work end-to-end with the new stuff and our docs refer to them. I'm very supportive of this PR but want to make sure we have some sort of continuity when moving towards the new object model. |
Fixes #582 |
remove API groups: - channels.knative.dev - feeds.knative.dev - flows.knative.dev Remove code and dependencies that depended directly on those API groups. This includes all Buses and EventSources. Where practical references to Channel and Subscription in channels.knative.dev have been updated to eventing.knative.dev. The e2e suite has been decimated and will need to be reestabilished.
95f5103
to
e3058cd
Compare
test/crd_checks.go
Outdated
@@ -23,8 +23,8 @@ import ( | |||
"fmt" | |||
"time" | |||
|
|||
eventingV1alpha1 "github.com/knative/eventing/pkg/apis/eventing/v1alpha1" | |||
eventingtyped "github.com/knative/eventing/pkg/client/clientset/versioned/typed/eventing/v1alpha1" | |||
flowsV1alpha1 "github.com/knative/eventing/pkg/apis/flows/v1alpha1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wrong?
test/image_paths.txt
Outdated
# The dispatcher used by the ClusterBus to dispatch messages between channels and subscriptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems to need to go away?
test/states.go
Outdated
@@ -16,7 +16,7 @@ limitations under the License. | |||
package test | |||
|
|||
import ( | |||
eventingV1alpha1 "github.com/knative/eventing/pkg/apis/eventing/v1alpha1" | |||
flowsV1alpha1 "github.com/knative/eventing/pkg/apis/flows/v1alpha1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems wrong?
The following is the coverage report on pkg/.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 Thanks for working through all this @scothis!
(I only reviewed hack/release.sh
)
@@ -23,23 +23,15 @@ readonly EVENTING_RELEASE_GCS | |||
readonly EVENTING_RELEASE_GCR | |||
|
|||
# Yaml files to generate, and the source config dir for them. | |||
declare -A COMPONENTS | |||
COMPONENTS["eventing.yaml"]="config" | |||
COMPONENTS["in-memory-channel.yaml"]="config/provisioners/in-memory-channel" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this duplicate the config that's already in eventing.yaml
, since that file contains this directory also? Then the release yaml will have two copies of the in-memory-channel objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grantr There will be three files produced for the release:
eventing.yaml
only knative eventingin-memory-channel.yaml
only the in-memory provisionerrelease.yaml
eventing.yaml + in-memory-channel.yaml
When you give a directory to ko, it will only build config files that are in that exact directory.
I ran this script with my own creds when I originally put up the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks, TIL. Apparently there's a --recursive
flag to kubectl
that selects the behavior I was expecting.
Minor preference for core.yaml
or eventing-core.yaml
over eventing.yaml
, but this is fine for now.
/hold cancel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the patten that serving uses, but it's easy enough to change.
Good with everything except the release script, letting @grantr unhold once release is right, everything else is lgtm. |
echo "# ${component}" >> ${yaml} | ||
cat ${component} >> ${yaml} | ||
done | ||
all_yamls+=(${yaml}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You missed tagging the images here (line 79 in the original code). This is a bug. :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adrcunha is it necessary to run tag_images_in_yaml
again? This step is only concatenating yamls generated above which were tagged here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. In line 62 you were tagging the component yamls. Now you're tagging the release yamls. Because on line 88 you're publishing all yamls (components and releases).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(sorry, this fell off my radar)
Proposed Changes
remove API groups:
Remove code and dependencies that depended directly on those API groups.
This includes all Buses and EventSources.
Where practical references to Channel and Subscription in
channels.knative.dev have been updated to eventing.knative.dev.
The e2e suite has been decimated and will need to be reestabilished.
Removed vendor dependencies for Samara (Kafka) and Cloud Pub/Sub.
Updated release script. Now publishes three artifacts:
Release Note