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

Added specifications for refactoring tendrl code #7

Merged
merged 3 commits into from
Dec 14, 2016

Conversation

shtripat
Copy link
Member

@shtripat shtripat commented Dec 1, 2016

This specification details the refactoring across the tendrl
modules to move major common code to common module.

Signed-off-by: Shubhendu shtripat@redhat.com

This specification details the refactoring across the tendrl
modules to move major common code to common module.

Signed-off-by: Shubhendu <shtripat@redhat.com>
@shtripat
Copy link
Member Author

shtripat commented Dec 1, 2016

Copy link
Contributor

@r0h4n r0h4n left a comment

Choose a reason for hiding this comment

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

Separate out refactoring of each component into individual specs, common, tendrl-gluster-integraton etc

@shtripat
Copy link
Member Author

shtripat commented Dec 2, 2016

@r0h4n I was of a thought that each module specific code changes would be created as individual github issues and this spec PR would be linked to all of them. So effectively this spec PR ends up into multiple github issues and multiple re-factoring code change PRs as a result.

@nthomas-redhat @brainfunked comments?? ^^

If everybody strongly feels, its required, I can create multiple ones (its just a copy paste of same content mostly ;))

Copy link
Contributor

@brainfunked brainfunked left a comment

Choose a reason for hiding this comment

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

Need implementation details regarding which specific flows need to be moved.

@@ -0,0 +1,206 @@
= Refactor modules like tendrl-gluster-integartion, tendrl-node-agent etc and
Copy link
Contributor

Choose a reason for hiding this comment

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

Line breaks do not work in titles, if I'm not mistaken. Shorten the main title. Add a subtitle if you wish to convey additional context.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. Will shorten the heading..


== Use Cases

None
Copy link
Contributor

Choose a reason for hiding this comment

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

The most important use case is that this enables commons to function as a library and the code becomes completely reusable by any component.

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. Would add the use case details.

@r0h4n
Copy link
Contributor

r0h4n commented Dec 2, 2016

@shtripat Specs needs to be per component and github issues should be against that component, It is difficult to track one spec spanning multiple components. Also that way component owners can review those specs without needing much insight on other components apart from making sure the impact on other components is documented

@brainfunked
Copy link
Contributor

@r0h4n Specifications are tendrl-global. They can affect multiple components. The specification should explicitly point out the affected components and the changes required across them. Issues from the specification can be created across multiple components. The issues detail out the exact implementation changes required in each of the components.

Basically, specifications are our tendrl project design. Issues are per component implementation details.

@shtripat
Copy link
Member Author

shtripat commented Dec 2, 2016

@brainfunked Agree. Even I feel this specification could be single and we can add all the required details here and then link the github issues to this from different modules of tendrl.
I would prefer retaining this as single spec.

Signed-off-by: Shubhendu <shtripat@redhat.com>
@nnDarshan
Copy link
Contributor

@shtripat Can you please mention about moving the general purpose utilities like command executor, package installer, service manager etc to common in this spec.
I see you have already mentioned about utils, but they are more specific to flows, manager etc.

@shtripat
Copy link
Member Author

@nnDarshan regarding your comment, I think enough details are available under "Proposed changes" and "Work Items". These section talk about manager, persister changes etc in detail. Anything specific you need?

@nnDarshan
Copy link
Contributor

@shtripat Can you please mention about general purpose utilities like command executor, package installer, service manager.

Copy link
Contributor

@nthomas-redhat nthomas-redhat left a comment

Choose a reason for hiding this comment

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

overall LGTM if the comments are addressed


* https://github.com/Tendrl/common/issues/60

* https://github.com/Tendrl/common/issues/62
Copy link
Contributor

Choose a reason for hiding this comment

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

Have a mention about issue 60 as well in the document. That means common module specific logging and configuirations will be removed. This has a documentaion impact as well

Also you need to create individual issues per component(changes occuring) and link it here

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@shtripat can you please include: Tendrl/commons#80. Sorry for informing you late.

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

Created individual issues for different modules and
referred into specification.

Signed-off-by: Shubhendu <shtripat@redhat.com>
Copy link
Contributor

@nthomas-redhat nthomas-redhat left a comment

Choose a reason for hiding this comment

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

Looks good to me

@r0h4n r0h4n merged commit eacf352 into Tendrl:master Dec 14, 2016
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