Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
initial cockroachdb-operator charm for review #1
base: master
Are you sure you want to change the base?
initial cockroachdb-operator charm for review #1
Changes from 4 commits
d0fad56
6b37ae4
95c61eb
d9811f0
e6741a9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation in this file is inconsistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a nice file or module name to read/write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I rushed a bit with names. I think names like TCPLoadBalancer and TCPServer (or TCPBackend) would be more comprehensive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything here is from CochroachDB, so either we consistently prefix them after the database, which doesn't seem so nice since the class itself already gives these names context, or we avoid the term in all of them. My preference is for the latter.
The first one might be database_started, or daemon_started, etc, depending on what it means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed,
daemon_started
or evendaemonized
would be better.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should probably be
_stored
instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit unexpected to see all of these under this class. What's the rationale?
They feel more like properties of the overall charm code than this type in particular. If they indeed are supposed to be changeable somehow depending on this type, though, then they likely shouldn't look like constants. But happy to discuss further looking at established conventions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to associate them with a particular type until I needed them elsewhere to avoid having globals. I aim to split out the code that mutates the machine state into a separate component - it might be that the need to pull those attributes out will arise at that point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss further if it's a problem.
Otherwise, I moved some "constants" to a separate type:
e6741a9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a better type name for that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed in #1 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why all caps when this is a local variable?
What if resource_path is not None? The variable will be unset and the logic below will break down, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to indicate that it is meant to be a constant on purpose.
If a resource is not provided at the deployment time or later, and an attempt is made to retrieve it, a
ModelError
will be raised which will result inNone
being assigned toresource_path
(so we can just try and download a binary instead). Otherwise, it will be a valid file system path.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stopped that initial review here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New changes:
e6741a9