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

Definitions #304

Merged
merged 22 commits into from Jan 14, 2020
Merged

Definitions #304

merged 22 commits into from Jan 14, 2020

Conversation

TheFoxAtWork
Copy link
Collaborator

@TheFoxAtWork TheFoxAtWork commented Dec 5, 2019

Checked with @SantiagoTorres looks good so far

Copy link
Collaborator

@lumjjb lumjjb left a comment

Thanks for creating this. Content is great! I've added a couple comments just around wording and nits.

Copy link
Collaborator Author

@TheFoxAtWork TheFoxAtWork left a comment

changes requested except one have been made

@TheFoxAtWork
Copy link
Collaborator Author

TheFoxAtWork commented Dec 16, 2019

@lumjjb ready for ur review....

Copy link
Collaborator

@lumjjb lumjjb left a comment

Replied to some of the changes - I think it looks good to me after the minor changes. If @SantiagoTorres can do a review of the deltas and LGTM as well. And let's add this on the agenda of one of the meetings to share this new doc as well!

@SantiagoTorres
Copy link
Collaborator

SantiagoTorres commented Dec 17, 2019

I like this, although I'm also not fully convinced if the word defeat is the term we'd like. In one hand I like that it does carries some strength/determination, but on the other I think it's not entirely clear what it means at first glance...

Other than that, pretty solid work @TheFoxAtWork !

Copy link
Collaborator Author

@TheFoxAtWork TheFoxAtWork left a comment

pushed changes requests please review @lumjjb @SantiagoTorres

lumjjb
lumjjb approved these changes Dec 19, 2019
Copy link
Collaborator

@lumjjb lumjjb left a comment

LGTM

@SantiagoTorres
Copy link
Collaborator

SantiagoTorres commented Dec 19, 2019

I can't approve, but LGTM as well :)

@lumjjb
Copy link
Collaborator

lumjjb commented Dec 19, 2019

@ultrasaurus @pragashj @dshaw This looks good to merge.. any additional comments?

@lumjjb
Copy link
Collaborator

lumjjb commented Jan 3, 2020

Just waiting for additional comments from co-chairs. can discuss this at next meeting and merge this probably

@dshaw dshaw self-requested a review Jan 7, 2020
dshaw
dshaw approved these changes Jan 7, 2020
Copy link
Collaborator

@dshaw dshaw left a comment

Looks good.

@ultrasaurus
Copy link
Member

ultrasaurus commented Jan 7, 2020

This is fabulous. Just one formatting issue before merging -- pls make the sections in the doc be the same order as the index. Looks like that is alphabetical, which seems like a fine approach to me.

@TheFoxAtWork
Copy link
Collaborator Author

TheFoxAtWork commented Jan 8, 2020

@ultrasaurus made the changes u requested. @SantiagoTorres also added in the link to the definitions on the readme under compromises

@ultrasaurus
Copy link
Member

ultrasaurus commented Jan 14, 2020

Looks great -- thanks @TheFoxAtWork !

@ultrasaurus ultrasaurus merged commit 8352673 into cncf:master Jan 14, 2020
1 check passed
@TheFoxAtWork TheFoxAtWork deleted the definitions branch Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants