letsencrypt / boulder

An ACME-based certificate authority, written in Go.
Mozilla Public License 2.0
5.21k stars 607 forks source link

Remove feature flags that have been activated in production #2692

Closed cpu closed 6 years ago

cpu commented 7 years ago

All of the above are activated in production and the feature flags can be retired.

klebervirgilio commented 7 years ago

is it something a novice could do?

cpu commented 7 years ago

@klebervirgilio Likely! It should be primarily an exercise in removing code & updating references. Are you interested in trying?

klebervirgilio commented 7 years ago

Sure! Any directions?

cpu commented 7 years ago

Sure! Any directions?

Thanks for the interest!

If this is your first foray into the codebase I would recommend starting off by getting Boulder installed locally (I definitely recommend the quickstart approach w/ Docker). You should confirm that you can issue against your local Boulder using Certbot (These certbot docs may help). Pay particular attention to FAKEDNS during the Boulder setup. Feel free to come by #letsencrypt-dev on Freenode IRC if you need help! After you can issue a test certificate locally you're in a good place to start this ticket knowing you can test along the way :-)

You'll also want to check out the contributing docs for Boulder to understand the review process. This is also where we explain feature flags which will be helpful knowledge for this cleanup PR.

For starting this issue I might begin with features.go and removing one of the flags (say, IDNASupport). This will break all of the references to it (for example, in the PA code) and you can start cleaning up the errors one by one until you can issue a test certificate again. Most of the flag references will be an if statement and the goal here will be to delete the if and always use the behaviour as if the flag were present & enabled, deleting the logic for the case where the flag was off.

If you want to submit a partial PR with just one flag removed as a starting point we can help you iterate faster.

klebervirgilio commented 7 years ago

@cpu Thank you so much!! I'm already working on that. it shouldn't take long. :)

cpu commented 7 years ago

@klebervirgilio One other "gotcha" I forgot to mention - the feature flags are implemented partially using a stringer codegen and part of our build cycle checks that all generated code checked into the repo matches what is generated fresh during CI.

The tl;dr is that as part of your work, whenever you change something in features.go you will want to run docker-compose run boulder go generate ./features/... to rerun the stringer generation inside the docker container. That will modify the features_string.go file that you check in alongside your other diffs. Using the Docker container to run go generate instead of doing it from your host machine means you won't have to worry about matching versions up with CI.

klebervirgilio commented 7 years ago

Got you! thanks!

jsha commented 7 years ago

Additional flags that can be removed:

rolandshoemaker commented 7 years ago

I thought we had individual issues for this but apparently not.

jsha commented 7 years ago

@klebervirgilio as a heads-up, I added a couple more entries to the list above

klebervirgilio commented 7 years ago

@jsha as soon as #3111 get merged, we can close this issue?

rolandshoemaker commented 6 years ago

Closing this out since it's slightly ambiguous and is something we should be doing regularly anyway as part of maintaining the cleanliness of the repo.