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

Split mongod and mongos, removed mongodb meta state #60

Closed
wants to merge 1 commit into from
Closed

Split mongod and mongos, removed mongodb meta state #60

wants to merge 1 commit into from

Conversation

sethcenterbar
Copy link

references #56

So.. this is my first stab at it, attempting to follow your comments. I know it's not the most perfect solution, but it was the quickest way to separate them. I'm wondering if it would be best to leave the mongodb metastate as the installation of the server, and then have mongod and mongos simply configure and turn on those services. Thoughts @noelmcloughlin?

@noelmcloughlin
Copy link
Member

noelmcloughlin commented Mar 23, 2019

It depends. One approach suggests mongodb and mongodb.clean metastate should handle everything (mongos, mongod, bi, roto3t) but the modularity you are introducing allows pick & choose. That is certainly consistent and perhaps solution of least surprise (a good thing).

The other approach suggests mongodb and mongodb.clean metastate handle the most common use case - namely mongod - on basis newbies/busy people get what they expected and nothing else

I would lean towards 1st option - the README is there for a reason.

I'll do some testing during the week. thanks.

@sethcenterbar
Copy link
Author

Ok. You'll find in my pull request that I opted for there to be no mongodb.init, and assumed the user would just include the metastates they wanted. So, basically neither of the two things you just said 😆 Oops!

@noelmcloughlin
Copy link
Member

No problem - If we want a mongodb.init it can be added as afterthought.
Don't forget to update the README.
I'll test next week and review the PR. thx

@noelmcloughlin
Copy link
Member

Hi @sethcenterbar

I got some time to check the PR now - there are some issues. Here are a few suggestions.

  1. In mongodb/mongod directory change all occurrences of mongodb server to mongodb mongod.
  2. In mongodb/mongos directory change all occurrences of mongodb server to mongodb mongos.
  3. You could safely delete all directories named depreciated I think.
  4. Fix mongod/init.sls i.e. change server to mongod
  5. Fix mongos/init.sls i.e. change server to mongos.
  6. Maybe defaults.yaml should be tidied up similarly. The mongodb.server stuff could be moved to mongodb.system since "server" is defunct. something like this ...
mongodb:
  system:
     ... no change here .. etc  ...
    package: mongodb-org      ##<<< mongodb.server.stuff moved here.
    version: 4.0
    use_repo: False
    use_archive: False
    use_schema: False
    disable_transparent_hugepages: true
    binpath: /usr
    user: mongodb
    group: mongodb

@noelmcloughlin noelmcloughlin self-requested a review April 1, 2019 21:58
Copy link
Member

@noelmcloughlin noelmcloughlin left a comment

Choose a reason for hiding this comment

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

Good effort - few changes suggested. Once these are addressed we can test further.

@sethcenterbar
Copy link
Author

@noelmcloughlin , I really want to be able to help with this. I'm going to take some time to learn kitchen and try to understand the testing process better so I don't have to bounce every attempt off of you.

@noelmcloughlin
Copy link
Member

I don't mind, learning kitchen is not necessary since you can just do testing in a local VM. This is your first stab at Salt so bounce off me all you want. There is saltstack community on Slack too where you can get advice. We're open!!

@noelmcloughlin
Copy link
Member

Hi @sethcenterbar
Did you get any further with this? I have two other PRs to get merged before/after yours.

@sethcenterbar
Copy link
Author

sethcenterbar commented May 7, 2019 via email

@noelmcloughlin
Copy link
Member

It would be great to get this tested. The improvements in this PR are needed imho.

@noelmcloughlin
Copy link
Member

replaced by #83

@pull-assistant
Copy link

Score: 1.00

Best reviewed: commit by commit


Optimal code review plan

     Split mongod and mongos, removed mongodb meta state

Powered by Pull Assistant. Last update 410dec0 ... 410dec0. Read the comment docs.

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.

2 participants