-
Notifications
You must be signed in to change notification settings - Fork 4
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
Cleanup autogenerated server vars #268
Conversation
…var not found error
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.
Thank you!!! This has been a long time coming, great to see it finally happen.
It looks like there are a few vars that could also be removed from here? https://github.com/civiform/cloud-deploy-infra/blob/main/cloud/aws/templates/aws_oidc/variable_definitions.json#L44
@bion what vars in particular? I'm removing the line you linked to here |
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.
Looks good! I wonder if some of those marked as "used elsewhere" could be refactored later, e.g. AWS_SES_SENDER is really SENDER_EMAIL_ADDRESS, can we just use AWS_SES_SENDER everywhere instead of SENDER_EMAIL_ADDRESS? But definitely not needed for this one.
Good idea, I can make an issue to follow up with that. |
This reverts commit 01dba6c.
This PR removes variables that are generated/referenced by server variable auto-generation now and are no longer needed. It also removes a method (
overwrite_checkout_file
) that was used to rollout server var auto-generation and is no longer needed.I mostly focused on removing vars from the shared files and AWS since those are our active deployments, but I also removed some vars from Azure that are clearly no longer needed.
The variables removed in this PR are listed in this doc and grouped by the reason it is safe to remove them. There are 54 of them (!!). I erred on the side of caution when deciding what to remove, but I am not familiar with all of these variables and would appreciate a second eye.
Testing:
New deployments: I ran
bin/setup
with the changes on this branch and confirmed it deployed successfully and basic functionality worked.Existing deployments: I ran
bin/setup
using the code on main thenbin/deploy
using the code on this branch and confirmed deployment was successful and nothing obvious broke or changed. I compared the admin settings panel pages and made sure all values were consistent. The only difference is theESRI_FIND_ADDRESS_CANDIDATES_URL
which previously defaulted to a value set in the deploy system, but will now be blank unless set in the config file.After this is merged, I'll test deploying it on staging to ensure login functionality isn't affected.
Closes: civiform/civiform#5249