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

Review docs (one isn't just a date bump!) #2227

Merged
merged 5 commits into from Jan 14, 2020
Merged

Review docs (one isn't just a date bump!) #2227

merged 5 commits into from Jan 14, 2020

Conversation

@issyl0
Copy link
Member

issyl0 commented Jan 13, 2020

Four of these are standard date bumps because the content is still OK.
But I'll copypaste the contents of 1e527ed here because it's the most
important.


terraboard: Bump review date but reduce time between reviews

  • There are some things here that I'm not completely comfortable with,
    but that require engineering effort to change. And an assurance that
    Terraboard is still being used. These things are:

    • We currently have our own Docker image for oauth2_proxy, based on
      the original creators of oauth2_proxy. Our repo (linked in the doc
      still) is archived, as is the upstream repo that we cribbed from. The
      current maintainers of oauth2_proxy are now on version 4.1.0 - but
      we're still on 2.2.0.
    • There's some work here not just on these docs but to stop us
      maintaining so much code ourselves when the new project maintainers
      now publish an official Docker image
      (https://quay.io/repository/pusher/oauth2_proxy).
    • Also, anecdotal evidence suggests that devs get confused when trying
      to restart the Smokey process on the monitoring box - there's a
      PostgreSQL database for Terraboard on that box running as the smokey
      user?!
  • Once these issues are resolved, I'll be happy to bump the review date
    up to 6 months again, but right now referencing archived repos doesn't
    look great to outsiders looking in, or to people trying to find more
    about how this stuff works!

issyl0 added 4 commits Jan 13, 2020
@karlbaker02 karlbaker02 temporarily deployed to govuk-developer-docs-f-pr-2227 Jan 13, 2020 Inactive
Copy link
Contributor

ChrisBAshton left a comment

Thanks for the PR!

I don't know if it's worth giving the green light to use-terraboard-to-monitor-terraform-state.html.md whilst acknowledging that it needs a rewrite, it feels like we're just kicking this can down the road by a month. I think I'd sooner this kept coming up in the manual spaniel reminders until we set time aside to amend it properly 🤔 what do you think?

Appreciate the rest of the date bumps 👍 nice to get that list down!

@issyl0

This comment has been minimized.

Copy link
Member Author

issyl0 commented Jan 13, 2020

I don't know if it's worth giving the green light to use-terraboard-to-monitor-terraform-state.html.md whilst acknowledging that it needs a rewrite, it feels like we're just kicking this can down the road by a month. I think I'd sooner this kept coming up in the manual spaniel reminders until we set time aside to amend it properly 🤔 what do you think?

I was umming and ahhing about this exact thing. But I decided to go with it and write the problems down in the commit message/PR so that someone could refer back in the future and deal with the problems at a more convenient time (I don't have any time at the moment to get stuck into doing more investigation than I already have done, really).

@ChrisBAshton

This comment has been minimized.

Copy link
Contributor

ChrisBAshton commented Jan 13, 2020

That makes sense - my biggest concern is that your commit message will never be read again. (Not sure about other devs, but when I review a doc I just read the doc, I won't typically dig into its commit history for any pointers from previous devs).

Are there places in the use-terraboard-to-monitor-terraform-state.html.md file where you can write something like this...?

The following section may be out of date because of X. See [PR #2227](https://github.com/alphagov/govuk-developer-docs/pull/2227) for details.
- There are some things here that I'm not completely comfortable with,
  but that require engineering effort to change. And an assurance that
  Terraboard is still being used. These things are:
  - We currently have our own Docker image for oauth2_proxy, based on
    the original creators of oauth2_proxy. Our repo (linked in the doc
    still) is archived, as is the upstream repo that we cribbed from. The
    current maintainers of oauth2_proxy are now on version 4.1.0 - but
    we're still on 2.2.0.
  - There's some work here not just on these docs but to stop us
    maintaining so much code ourselves when the new project maintainers
    now publish an official Docker image
    (https://quay.io/repository/pusher/oauth2_proxy).
  - Also, anecdotal evidence suggests that devs get confused when trying
    to restart the Smokey process on the monitoring box - there's a
    PostgreSQL database for Terraboard on that box running as the `smokey`
    user?!
- Once these issues are resolved, I'll be happy to bump the review date
  up to 6 months again, but right now referencing archived repos doesn't
  look great to outsiders looking in, or to people trying to find more
  about how this stuff works!
Copy link
Contributor

ChrisBAshton left a comment

LGTM 👍 thanks Issy!

@issyl0 issyl0 merged commit 5ce8d7b into master Jan 14, 2020
1 check passed
1 check passed
continuous-integration/jenkins/branch This commit looks good
Details
@issyl0 issyl0 deleted the review-docs branch Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.