Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

Factorio version bump + configurable port. #547

Merged
merged 4 commits into from
Mar 15, 2017
Merged

Factorio version bump + configurable port. #547

merged 4 commits into from
Mar 15, 2017

Conversation

gtaylor
Copy link
Collaborator

@gtaylor gtaylor commented Feb 4, 2017

This PR includes two commits:

@k8s-ci-robot
Copy link
Contributor

Hi @gtaylor. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 5, 2017
@linki
Copy link
Contributor

linki commented Feb 5, 2017

@gtaylor, I think this isn't going to work. FACTORIO_PORT tells factorio to listen on that port inside the container. But the targetPort of your service is factorio which is still hard-coded to 34197 in the Deployment spec.

https://github.com/gtaylor/charts/blob/0be377d5f5dfe5e9e8c5edfded50b30807e43141/stable/factorio/templates/deployment.yaml#L75

My goal with the configurable port was to make the chart work better with factorio's server registry. Having a separate load balancer IP doesn't work as factorio registers itself with its node IP address. Similar behaviour for the port: whatever port factorio is told to listen on gets registered with the registry, so it must also listen on that port on the external address. Feel free to correct me if I'm wrong.

@linki
Copy link
Contributor

linki commented Feb 5, 2017

Changing containerPort in the deployment spec to {{ .Values.factorioServer.port }} should work.

@linki
Copy link
Contributor

linki commented Feb 5, 2017

Seems like I'm very wrong. Using arbitrary NodePorts works just fine with the server browser. I don't even have to open any ports in the firewall. Does this relate to their NAT punching? (https://www.factorio.com/blog/post/fff-143)

Anyways, I would leave out the configurable port commit for now. Your version 0.1.5 works fine.

@gtaylor
Copy link
Collaborator Author

gtaylor commented Feb 9, 2017

Configurable port could be useful for those (such as myself) who don't publish their server in the directory but want an alternate port.

@lachie83
Copy link
Contributor

@gtaylor @linki What's the consensus here? Can we proceed with the merge?

@lachie83
Copy link
Contributor

@k8s-bot ok to test

@lachie83
Copy link
Contributor

CI failing with the following W0213 19:57:24.138] Error: release factori-965 failed: error validating "": error validating data: expected type int, for field spec.ports[0].port, got string

@linki
Copy link
Contributor

linki commented Feb 16, 2017

@lachie83 @gtaylor https://github.com/gtaylor/charts/pull/4 should fix the CI build.

@linki
Copy link
Contributor

linki commented Feb 27, 2017

@gtaylor can you have peek at https://github.com/gtaylor/charts/pull/4 so we can merge this?

It just fixes an issue with the port being a string where it should be an int 😕

@lachie83 lachie83 modified the milestone: Milestone 4 Mar 9, 2017
@linki linki self-assigned this Mar 15, 2017
@linki
Copy link
Contributor

linki commented Mar 15, 2017

I can take care of it once I've got access to push here.

@linki
Copy link
Contributor

linki commented Mar 15, 2017

@gtaylor I fixed the issue with the port being a string and now the e2e tests pass.

@gtaylor
Copy link
Collaborator Author

gtaylor commented Mar 15, 2017

Sorry for the delay. LGTM!

@linki linki added UX reviewed lgtm Indicates that a PR is ready to be merged. and removed awaiting review labels Mar 15, 2017
@lachie83 lachie83 merged commit e6ebe2a into helm:master Mar 15, 2017
yanns pushed a commit to yanns/charts that referenced this pull request Jul 28, 2017
* Bump factorio imageTag from 0.14.21 to 0.14.22.

* Make factorio's game server port configurable.

* fix(factorio): change port from string to int
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. code reviewed lgtm Indicates that a PR is ready to be merged. UX reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants