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

Refactor core #154

Merged
merged 2 commits into from Jul 8, 2021
Merged

Refactor core #154

merged 2 commits into from Jul 8, 2021

Conversation

ankithans
Copy link
Member

@ankithans ankithans commented Jul 8, 2021

Description

Extract packages from starter and put them into charmil-core
Next Step would be to make a starter CLI able to run all these packages!
after that a starter template -> then CLI tool

I will provide documentation for all the packages, once i get them running in the example

@ankithans ankithans requested a review from a team July 8, 2021 08:50
@ankithans ankithans linked an issue Jul 8, 2021 that may be closed by this pull request
@ankithans ankithans added the enhancement New feature or request label Jul 8, 2021
@wtrocki
Copy link
Contributor

wtrocki commented Jul 8, 2021

@ankithans Starter have opiniated libraries from rhoas bringing them to core might be challenging.
IMHO It is way easier to figure out core/config etc. and swap it with what is there

#149

@wtrocki
Copy link
Contributor

wtrocki commented Jul 8, 2021

I thino this PR has tons of good changed:

  • Removes elements that are already in core
  • Swaps some elements with charmil

I will leave others for now as this PR might be too large to deal with all at the same time.

Copy link
Contributor

@wtrocki wtrocki left a comment

Choose a reason for hiding this comment

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

Good to go after cleanup and rebase.
World class job

@wtrocki
Copy link
Contributor

wtrocki commented Jul 8, 2021

@namit-chandwani we are missing only config

@ankithans
Copy link
Member Author

ankithans commented Jul 8, 2021

Removes elements that are already in core
Swaps some elements with charmil

yes it does these changes and next after this merge I will make these new features in core - running with starter example.

@wtrocki
Copy link
Contributor

wtrocki commented Jul 8, 2021

I will keep core small, well tested, well documented. PRs to core should be small and targeted.

@wtrocki
Copy link
Contributor

wtrocki commented Jul 8, 2021

Good to go!

@ankithans
Copy link
Member Author

ankithans commented Jul 8, 2021

so are you recommending to implement/test these additions to core in this PR? we can do that as well!
This may be better as we will not have any unused code in main. All things will be well tested and documented before adding to main.

@wtrocki
Copy link
Contributor

wtrocki commented Jul 8, 2021

so are you recommending to implement/test these additions to core in this PR? we can do that as well!
This may be better as we will not have any unused code in main. All things will be well tested and documented before adding to main.

No I recommend to minimize amount of untested code landing into core without issues/discussion.
Example: Colours - I do not see it being particularly useful but that is just my opinion.

Let's merge this but I want us to admin that we keeping core small/useful and functional

@ankithans ankithans merged commit d66af4d into main Jul 8, 2021
@ankithans ankithans deleted the refactor-core branch July 8, 2021 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pull pkg from starter to charmil core
2 participants