-
Notifications
You must be signed in to change notification settings - Fork 48
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
Parameterize # of GameServer Creation/Deletion #432
Conversation
@microsoft-github-policy-service agree company="Microsoft" |
196ef92
to
b401540
Compare
hey @nottagg, thanks for the PR! A couple of changes and we would be good to go! |
Hey @dgkanatsios, This is ready for review. I've also updated the original description to better reflect the changes made. I think things should further be refactored to eventually separate the type files (port_registry, config) from the controller folder into their own folder. But I won't include that onto this PR |
//If config is passed to a constructor, whatever fields constructor uses need to be defined explicitly | ||
//This does not pull values from operator.yaml like it does in main.go | ||
//For suite_test the env defaults should be used, defined in const above | ||
config := &Config{ |
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.
while this works, the Config struct defines some default values and it would be great to test them as well. Any chance we can use the same code we have in main.go? Like
cfg := &Config{}
if err := env.Parse(cfg); err != nil {
log.Fatal(err, "Cannot load configuration from environment variables")
}
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 might need to provide some dummy values for the ones that should not be empty, like THUNDERNETES_INIT_CONTAINER_IMAGE and THUNDERNETES_INIT_CONTAINER_IMAGE_WIN
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.
Latest commit uses the parse from main.go and filled in some defaults.
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.
great job @nottagg, thank you! |
Fixes #157
Felt like a pretty similar change to #419, so I figured I would take a look.
I did not make these env variables in the makefile. I could refactor the PR to do so if desired, but I figured it would be better to wait on a change fixing the dropped double quotes I faced in #419 . Unlike that one, these wouldn't require an int and string variable so it would be a little prettier. Combining these max add/delete into one variable may also be a refactor to consider. Thanks!