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

Add helm chart for the federator #1317

Merged
merged 5 commits into from
Jan 13, 2021
Merged

Add helm chart for the federator #1317

merged 5 commits into from
Jan 13, 2021

Conversation

akshaymankar
Copy link
Member

@akshaymankar akshaymankar marked this pull request as draft January 6, 2021 13:52
@akshaymankar akshaymankar marked this pull request as ready for review January 6, 2021 14:06
Copy link
Member

@jschaul jschaul left a comment

Choose a reason for hiding this comment

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

Some questions, but in general looks good and fine to merge if CI is green.

spec:
type: ClusterIP
ports:
# TODO: Do we name it something else to get the _correct_ SRV record?
Copy link
Member

@jschaul jschaul Jan 6, 2021

Choose a reason for hiding this comment

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

I don't quite understand this TODO comment. What does the kubernetes port name have to do with SRV records? Naming this port http seems fine to me. Do we use the port name somewhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

When we call this port http, kubernetes will create SRV record as _http._tcp.federator.<namespace>.cluster.local.
I assume for discovering remote federators, we will rely on an SRV record like _wire-federator._tcp.<domain>.
So, if we rename this port from http to wire-federator and configure domain as federator.<namespace>.cluster.local we will be able to test federation in same kubernetes cluster without having to mess with global DNS records. All of this is obviously not thought through, so I would much rather tackle it later. I will remove this TODO and write this information in some issue in JIRA.

internalPort: 8080

resources:
# FUTUREWORK: come up with numbers which didn't appear out of thin air
Copy link
Member

Choose a reason for hiding this comment

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

I think we can probably leave the resources block entirely empty: resources: {}.
But I'm also okay to follow the other charts and to make a change for all charts at once regarding resource usage, instead of only for the federator.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, let's do them all together.

@@ -9,3 +9,4 @@ tags:
team-settings: false
account-pages: false
legalhold: false
federator: false
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to disable installing this compoent by default, and enable it once the implementation is more "ready"? Or do you want to keep this false also going forward?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was thinking we shouldn't enable it by default at least until it is useful. Even when it is useful, maybe we shouldn't deploy it when federation is not enabled.

@akshaymankar akshaymankar merged commit 710efda into develop Jan 13, 2021
@akshaymankar akshaymankar deleted the federator/helm-chart branch January 13, 2021 15:35
@smatting smatting mentioned this pull request Jan 15, 2021
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