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

feat: add nginx container to serve robots.txt #3836

Merged
merged 2 commits into from
Sep 16, 2024
Merged

Conversation

rpcross
Copy link
Collaborator

@rpcross rpcross commented Sep 9, 2024

No description provided.

@rpcross
Copy link
Collaborator Author

rpcross commented Sep 9, 2024

I based this on Datatracker nginx setup. Does this look good for mail archive? I am not familiar with the $${keepempty} usage in nginx conf.

@jennifer-richards
Copy link
Member

jennifer-richards commented Sep 10, 2024

I am not familiar with the $${keepempty} usage in nginx conf.

This is a convention we're using to get around quoting issues. The issue is that kustomize does not support environment variable substitution, so we run its output through envsubst to do that for us. That will strip out any strings like $whatever, and the nginx conf needs those. The $${keepempty}whatever reduces down to $whatever in this process. The "keepempty" could be any string that's not an actual environment variable.

This may go away as we refactor how we deal with config / secrets in k8s as we might not need the envsubst step any more.

Copy link
Member

@jennifer-richards jennifer-richards left a comment

Choose a reason for hiding this comment

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

Sorry for the waffling, but it'd be best to move the main app container to the start of the containers list. That helps our k9s management interface show the right container for the pod in its summary view.

@rpcross rpcross merged commit 35fbb62 into ietf-tools:main Sep 16, 2024
1 check passed
@rpcross rpcross deleted the nginx-2 branch September 16, 2024 16:35
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