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

Create NewObject methods & improve (default) conf loading #180

Open
xlr-8 opened this issue Dec 4, 2017 · 3 comments
Open

Create NewObject methods & improve (default) conf loading #180

xlr-8 opened this issue Dec 4, 2017 · 3 comments

Comments

@xlr-8
Copy link
Contributor

xlr-8 commented Dec 4, 2017

Issue type

  • Enhancement

Summary

While reviewing https://github.com/cristim/autospotting/pull/175 I realised that we could create NewObject methods, in order to improve the way we instantiate objects such as:
https://github.com/cristim/autospotting/blob/2c9e019ffc2f7c2025236433f180d4e519a5294e/core/main.go#L64
https://github.com/cristim/autospotting/blob/2c9e019ffc2f7c2025236433f180d4e519a5294e/core/region.go#L316
https://github.com/cristim/autospotting/blob/2c9e019ffc2f7c2025236433f180d4e519a5294e/core/main.go#L20

Details

This would for example help for loading the default configuration and overiding it with special ASG configuration.

The default configuration could be loaded here: https://github.com/cristim/autospotting/blob/2c9e019ffc2f7c2025236433f180d4e519a5294e/core/main.go#L20
While the ASG configuration would be added here: https://github.com/cristim/autospotting/blob/2c9e019ffc2f7c2025236433f180d4e519a5294e/core/region.go#L296 - the call to load ASG (https://github.com/cristim/autospotting/blob/2c9e019ffc2f7c2025236433f180d4e519a5294e/core/region.go#L316) would be improved by the NewAutoscalingGroup method which would load the configuration for this ASG based on the default configuration of the region.

Instead of doing it later during the process here: https://github.com/cristim/autospotting/blob/2c9e019ffc2f7c2025236433f180d4e519a5294e/core/autoscaling.go#L176 as it doesn't make that much sense.

EDIT: updated links to replace master by a commit relevant at that time; otherwise the lines weren't matching anything meaningful anymore.

@cristim
Copy link
Member

cristim commented Feb 10, 2018

@xlr-8 I'd love to see a PR for this.

@xlr-8
Copy link
Contributor Author

xlr-8 commented Feb 15, 2018

Yeah me too, I've been off computer lately, sorry

@cristim
Copy link
Member

cristim commented Apr 28, 2019

I'm working on something like this in preparation for a new feature that touches this area. Stay tuned.

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

No branches or pull requests

2 participants