Skip to content
This repository was archived by the owner on Jul 11, 2023. It is now read-only.

Feature/bootstrap #4858

Merged
merged 2 commits into from
Jul 1, 2022
Merged

Feature/bootstrap #4858

merged 2 commits into from
Jul 1, 2022

Conversation

steeling
Copy link
Contributor

Remove redundant bootstrap config, and leverage a builder pattern we plan on implementing.

Part of #4818

@steeling steeling force-pushed the feature/bootstrap branch 2 times, most recently from 3233bba to 933f7f5 Compare June 29, 2022 19:32
@steeling steeling marked this pull request as ready for review June 29, 2022 19:35
@steeling
leverage builder pattern for building the bootstrap config
524a720 
Signed-off-by: Sean Teeling <seanteeling@microsoft.com>
@steeling steeling force-pushed the feature/bootstrap branch from 933f7f5 to 524a720 Compare June 29, 2022 19:39
draychev
draychev reviewed Jun 30, 2022
View reviewed changes
Copy link
Contributor

@draychev draychev left a comment

Choose a reason for hiding this comment

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

I left some minor comments around naming. I really care about readability. I think readability makes the project more approachable by new folks wanting to contribute. (And the rest of us reading this new code)

This looks great. Thanks for spearheading the effort. The effort you describe in the document here is ambitions and much needed. @eduser25 had started similar efforts and I have a few docs as well. I'd love to join the refactoring party.

I'd love to see more details in the doc around what specific changes you want to make. We could then divide and conquer. I'd love to help!

@steeling
address comments
d315e20 
Signed-off-by: Sean Teeling <seanteeling@microsoft.com>
@steeling
Copy link
Contributor Author

I left some minor comments around naming. I really care about readability. I think readability makes the project more approachable by new folks wanting to contribute. (And the rest of us reading this new code)

This looks great. Thanks for spearheading the effort. The effort you describe in the document here is ambitions and much needed. @eduser25 had started similar efforts and I have a few docs as well. I'd love to join the refactoring party.

I'd love to see more details in the doc around what specific changes you want to make. We could then divide and conquer. I'd love to help!

Sure, I'll be creating some specific bugs soon which will have more detail!

draychev
draychev suggested changes Jun 30, 2022
View reviewed changes
Copy link
Contributor

@draychev draychev left a comment

Choose a reason for hiding this comment

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

It'd be good to be more specific on builder and name it so it indicates what it's expected to build.

draychev
draychev approved these changes Jul 1, 2022
View reviewed changes
Copy link
Contributor

@draychev draychev left a comment

Choose a reason for hiding this comment

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

Thanks for starting this refactor @steeling

@draychev draychev merged commit 3bf989a into openservicemesh:main Jul 1, 2022
@jaellio
Copy link
Contributor

jaellio commented Jul 1, 2022

@draychev thank you for the review! Just a reminder to clean up/consolidate the commit message before merging

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@shashankram shashankram Awaiting requested review from shashankram

@snehachhabria snehachhabria Awaiting requested review from snehachhabria

@nojnhuh nojnhuh Awaiting requested review from nojnhuh

@jaellio jaellio Awaiting requested review from jaellio jaellio is a code owner

2 more reviewers

@trstringer trstringer trstringer approved these changes

@draychev draychev draychev approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@steeling @jaellio @trstringer @draychev