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

made InstanceConfig take service, cluster and instance as args #167

Merged
merged 5 commits into from
Jan 21, 2016

Conversation

mjksmith
Copy link
Contributor

InstanceConfig and it's children MesosServiceConfig and ChronosJobConfig now directly store service name, cluster and instance. This should enable some cleaner code in the future.

@solarkennedy
Copy link
Contributor

I believe this is a good thing, and will make our code simpler in general because we will need to just pass this object around and the metadata will come with it.

It also enables the possibility for us to set environment variables in each app with this metadata, which is kinda something I've wanted for a while.

I would like a second opinion from someone who knows more about OOP in python. @EvanKrall or @Rob-Johnson ?

@EvanKrall
Copy link
Member

Looking.

@mjksmith mjksmith force-pushed the PAASTA-966_generate_deployments_refactor branch from a159412 to 60ea622 Compare January 19, 2016 22:43
@mjksmith
Copy link
Contributor Author

I'll fix up the general_itests right now

@Rob-Johnson
Copy link
Contributor

I think this is fine - my only groan is about job_name & instance being used interchangeably in different parts of the codebase. I don't care which it is, just that it's consistent.

@solarkennedy
Copy link
Contributor

My opinion is: Call it instance, and only at the last second when we must interact with the clientlib/api call it by whatever it needs to be called (job, job_name, job_id, etc.)

@mjksmith mjksmith force-pushed the PAASTA-966_generate_deployments_refactor branch from b3990ee to c57c8fc Compare January 20, 2016 22:23
@mjksmith mjksmith force-pushed the PAASTA-966_generate_deployments_refactor branch from c57c8fc to 9443e45 Compare January 21, 2016 18:36
mjksmith added a commit that referenced this pull request Jan 21, 2016
…actor

made InstanceConfig take service, cluster and instance as args
@mjksmith mjksmith merged commit f624930 into master Jan 21, 2016
@mjksmith mjksmith deleted the PAASTA-966_generate_deployments_refactor branch January 21, 2016 18:36
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.

4 participants