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

Add dashboard role handler #543

Merged
merged 8 commits into from Mar 23, 2017

Conversation

imobachgs
Copy link
Contributor

@imobachgs imobachgs commented Mar 23, 2017

See https://trello.com/c/PJqM8x0T for details.

CaaSP deserves its own YaST package!

Among other things, our team is working hard to make YaST fulfill the requirements of the upcoming Containers as a Service Platform (CaaSP) product. As part of this effort, we have added some new features, discovered (and fixed) some bugs, improved documentation and so on.

We've also added some specific CaaSP code, so during this sprint we've decided to create a new dedicated package (yast2-caasp) and move the it there.

Currently it only contains system role handlers and some additional documentation, but most probably we will add some stuff within the upcoming months.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 16.352% when pulling 661c867 on imobachgs:add-dashboard-role-handler into 2fb2ee2 on yast:SLE-12-SP2-CASP.

Copy link
Contributor

@teclator teclator left a comment

Choose a reason for hiding this comment

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

LGTM, thnx for refactoring it a bit 👍

@imobachgs
Copy link
Contributor Author

Thanks for the review! :)

@imobachgs imobachgs merged commit c2280d9 into yast:SLE-12-SP2-CASP Mar 23, 2017
# ------------------------------------------------------------------------------

require "yast"
require "installation/cfa/salt"
Copy link
Member

Choose a reason for hiding this comment

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

why it is needed here??? It is for worker role only, so it should be in that file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I'll fix it (without version bumping) and anyway I'll remove it when merging into master.

@kobliha
Copy link
Member

kobliha commented Mar 23, 2017

@imobachgs Just a side note: This really looks like something not generic enough, something that probably belongs to a CaaSP-specific package. But it's just an opinion to consider...

@imobachgs imobachgs deleted the add-dashboard-role-handler branch March 23, 2017 16:41
@imobachgs
Copy link
Contributor Author

@kobliha Yes, it sounds good: we could move those roles handlers to a yast-caasp package. And we should think also about more code that could be included in that specific package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants