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

Address grantr feedback on #149 #151

Merged
merged 10 commits into from
Mar 11, 2021

Conversation

evankanderson
Copy link
Member

@evankanderson evankanderson commented Jun 17, 2020

See #149

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 17, 2020
@knative-prow-robot knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 17, 2020
@grantr
Copy link
Contributor

grantr commented Jun 17, 2020

This PR should be rebased onto master. Only 4aa47da is new; the other commits were merged in #149.

Copy link
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

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

Thanks @evankanderson! This resolves all my previous comments with the exception of #149 (comment). I created #152 to track that.

/lgtm

REPOSITORY-GUIDELINES.md Outdated Show resolved Hide resolved
REPOSITORY-GUIDELINES.md Outdated Show resolved Hide resolved
REPOSITORY-GUIDELINES.md Outdated Show resolved Hide resolved
REPOSITORY-GUIDELINES.md Outdated Show resolved Hide resolved
REPOSITORY-GUIDELINES.md Outdated Show resolved Hide resolved
REPOSITORY-GUIDELINES.md Outdated Show resolved Hide resolved
@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2020
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 18, 2020
Copy link
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2020
@evankanderson
Copy link
Member Author

/assign @pmorie

I think this is basically clarifications, but it needs steering to approve since it's in the top-level. Since you've been driving these discussions, assigning to you.

@vaikas
Copy link
Contributor

vaikas commented Mar 1, 2021

@pmorie just a ping if there's any changes still required here or if we're good to go.

@pmorie
Copy link
Member

pmorie commented Mar 2, 2021

looking at this now, my sincere apologies for latency

Copy link
Member

@pmorie pmorie left a comment

Choose a reason for hiding this comment

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

Looks great, just a couple tiny little nits, wdyt?

entire duration. Promotion from the sandbox into core will be handled on a
case-by-case basis by joint decision of the Steering Committee and the Technical
Oversight Committee.
the core; in most cases, projects in the sandbox will remain there for the
Copy link
Member

Choose a reason for hiding this comment

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

minor nit, I think i would prefer 'the knative github org' to 'the core', because 'the core' makes people feel all kinds of things

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Removed a few other places, but it starts at the top of the document, so there are cases in some parts of the doc still where it makes sense and is less likely to have feels.

networking or eventing integrations. In the event that a working group wants a
project in sandbox to be considered for transfer to the `knative` org, the
request will be considered on a case-by-case basis by joint decision of the
Steering Committee and the Technical Oversight Committee.
Copy link
Member

Choose a reason for hiding this comment

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

I think now that KTC would also have a say, yes?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about this -- I think the KTC would have a say as to whether it was part of any conformance spec, but not necessarily the location of the code.

@vaikas
Copy link
Contributor

vaikas commented Mar 4, 2021

@duglin @ronavn @bsnchan
To be discussed on 2021-03-04 meeting. Heads up so you get a chance to take a looksie.

@bsnchan
Copy link
Contributor

bsnchan commented Mar 5, 2021

TM committee spoke about this issue at our TM meeting on March 4th - we decided that the TM committee only really care about the "specs" repo and that structure of repos/ref implementation is out of scope for TM committee.

cc @knative/trademark-committee

Base automatically changed from master to main March 8, 2021 17:39
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2021
@pmorie
Copy link
Member

pmorie commented Mar 11, 2021

/approve
/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2021
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pmorie

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2021
@knative-prow-robot knative-prow-robot merged commit 322ed37 into knative:main Mar 11, 2021
Copy link
Member Author

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

(Forgot to send these comments, but wanted to enter them for the record.)

networking or eventing integrations. In the event that a working group wants a
project in sandbox to be considered for transfer to the `knative` org, the
request will be considered on a case-by-case basis by joint decision of the
Steering Committee and the Technical Oversight Committee.
Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about this -- I think the KTC would have a say as to whether it was part of any conformance spec, but not necessarily the location of the code.

entire duration. Promotion from the sandbox into core will be handled on a
case-by-case basis by joint decision of the Steering Committee and the Technical
Oversight Committee.
the core; in most cases, projects in the sandbox will remain there for the
Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Removed a few other places, but it starts at the top of the document, so there are cases in some parts of the doc still where it makes sense and is less likely to have feels.

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

Successfully merging this pull request may close these issues.

8 participants