Skip to content

CronJobSource uses fixed names, rather than GenerateName#1640

Merged
knative-prow-robot merged 16 commits into
knative:masterfrom
Harwayne:cronjob-fixed
Aug 6, 2019
Merged

CronJobSource uses fixed names, rather than GenerateName#1640
knative-prow-robot merged 16 commits into
knative:masterfrom
Harwayne:cronjob-fixed

Conversation

@Harwayne
Copy link
Copy Markdown
Contributor

@Harwayne Harwayne commented Aug 5, 2019 •
edited
Loading

Fixes #1250

Proposed Changes

  • CronJobSource uses fixed names for its generated Deployment and EventTypes.

Release Note

CronJobSource will delete its old receive adapter Deployment only if the CronJobSource
is Ready=True. If the CronJobSource never reaches Ready=True, then a subsequent 
release will ignore any old GenerateName based receive adapter Deployments. They
will be garbage collected when the CronJobSource itself is deleted. The same is true
for its old EventType.

@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 5, 2019
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 5, 2019
@knative-prow-robot knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 5, 2019
@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Aug 5, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Harwayne

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 Aug 5, 2019
@knative-prow-robot knative-prow-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 Aug 5, 2019
@Harwayne Harwayne marked this pull request as ready for review August 5, 2019 22:39
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 5, 2019
nachocano
nachocano reviewed Aug 5, 2019
View reviewed changes
Comment thread pkg/reconciler/cronjobsource/cronjobsource.go Outdated
nachocano
nachocano reviewed Aug 5, 2019
View reviewed changes
Comment thread pkg/reconciler/cronjobsource/cronjobsource.go
ra, err := r.KubeClientSet.AppsV1().Deployments(src.Namespace).Get(expected.Name, metav1.GetOptions{})
if apierrors.IsNotFound(err) {
ra, err = r.KubeClientSet.AppsV1().Deployments(src.Namespace).Create(expected)
msg := "Deployment created"
Copy link
Copy Markdown
Contributor

@nachocano nachocano Aug 5, 2019

Choose a reason for hiding this comment

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

no need for this msg... just use it in the Eventf call. Or use it in both places

Copy link
Copy Markdown
Contributor Author

@Harwayne Harwayne Aug 5, 2019

Choose a reason for hiding this comment

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

The reason why I added this is I couldn't find a way to get it to look nice otherwise. Initially I had:

r.Recorder.Eventf(src, corev1.EventTypeNormal, cronJobSourceDeploymentCreated, "Deployment created: error %v", err)

Which looked fine when there was an error, but looked bad when there wasn't (Deployment created: error <nil>). I like adding the error keyword, which is why I ended up with the current code. Overall, I want two distinct messages, "Deployment created" and "Deployment created, error: %v". Do you have a suggestion on how to accomplish that?

Copy link
Copy Markdown
Contributor

@nachocano nachocano Aug 5, 2019

Choose a reason for hiding this comment

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

sorry, my bad... Misread the code, they are indeed different logs...

nachocano
nachocano reviewed Aug 5, 2019
View reviewed changes
Comment thread pkg/reconciler/cronjobsource/cronjobsource.go Outdated
@knative-metrics-robot
Copy link
Copy Markdown

knative-metrics-robot commented Aug 5, 2019

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/cronjobsource/cronjobsource.go 69.0% 67.3% -1.7

@nachocano
Copy link
Copy Markdown
Contributor

nachocano commented Aug 5, 2019

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2019
@knative-prow-robot knative-prow-robot merged commit 990a966 into knative:master Aug 6, 2019
@Harwayne Harwayne deleted the cronjob-fixed branch September 17, 2019 20:32
pierDipi added a commit to pierDipi/eventing that referenced this pull request Mar 14, 2022
@pierDipi @aliok
Different cron times for branches for periodics (knative#1639) (knati…
cdce66e 
…ve#1640)

* Different cron times for branches

* Wrap function invocations in quotes

* Add instructions

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Co-authored-by: Ali Ok <aliok@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@grantr grantr Awaiting requested review from grantr

@n3wscott n3wscott Awaiting requested review from n3wscott

1 more reviewer

@nachocano nachocano nachocano left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@nachocano nachocano

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Fixed names rather than Generate Name

5 participants

@Harwayne @knative-prow-robot @knative-metrics-robot @nachocano @googlebot