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 Prout to guardian/story-packages #187

Merged
merged 2 commits into from
Sep 15, 2023
Merged

Add Prout to guardian/story-packages #187

merged 2 commits into from
Sep 15, 2023

Conversation

rtyley
Copy link
Member

@rtyley rtyley commented Sep 15, 2023

Why are you doing this?

Prout tells you when your pull request is actually deployed - or warns you if it's overdue, and should have been deployed by now.

Other examples of configuring Prout:

Interestingly, as the deploy of #184 did 'succeed', Prout wouldn't have given an Overdue warning - but would at least have prompted a good moment to test the PROD server.

image

This is prompted by the outage caused by #184,
where new PROD instances of https://packages.gutools.co.uk/ couldn't
start up. Although more specific testing would catch this issue before
it went live, as a last resort, it's good to know that a pull request
has _not_ gone live after merging, and that's one of the things
Prout can help with - posting a 'OVERDUE' message against the PR.

See also:

* https://github.com/guardian/prout
* https://www.theguardian.com/info/developer-blog/2015/feb/03/prout-is-your-pull-request-out
* Other examples of configuring Prout:
  * guardian/mobile-apps-api#2507
  * guardian/apple-news#232
@rtyley rtyley requested a review from Georges-GNM September 15, 2023 12:58
Copy link
Contributor

@Georges-GNM Georges-GNM left a comment

Choose a reason for hiding this comment

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

Think this looks good looking at other examples and the prout repo 👍 Just a couple of comments/questions more for my understanding than anything else!

build.sbt Outdated Show resolved Hide resolved
Resolver.typesafeRepo("releases")
)
) ++ Resolver.sonatypeOssRepos("releases")
Copy link
Contributor

Choose a reason for hiding this comment

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

From a quick look around, it sounds like this is the right/required way to declare this plugin so I'm curious as to why it wasn't the case already!

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, there are currently 24 repos still using the deprecated syntax! Only 21 with the updated syntax, eg guardian/frontend#25803.

@@ -53,6 +55,10 @@ resolvers ++= Seq(
Resolver.file("Local", file(Path.userHome.absolutePath + "/.ivy2/local"))(Resolver.ivyStylePatterns)
)

buildInfoPackage := "app"
buildInfoOptions += BuildInfoOption.BuildTime
buildInfoKeys += "gitCommitId" -> env.getOrElse("GITHUB_SHA", "Unknown")
Copy link
Contributor

@Georges-GNM Georges-GNM Sep 15, 2023

Choose a reason for hiding this comment

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

Just wondering about this, if a build ends up in prod, isn't it pretty much necessarily going to have github info? i.e. could env ever not have a Github_SHA?
Having said that, never a bad shout to have a fallback.

Copy link
Member Author

Choose a reason for hiding this comment

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

The build info stuff runs in Dev, on our laptops (which won't have GITHUB_SHA!), as well as in CI - so we need to have a fallback for when we're running on locally!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh of course, this still gets evaluated in those environments 🤦
Yep, the fallback is definitely required then!

@rtyley rtyley merged commit 2ed6d42 into main Sep 15, 2023
@rtyley
Copy link
Member Author

rtyley commented Sep 15, 2023

Looks like the tests in story-package are a little flakey:

image

@rtyley
Copy link
Member Author

rtyley commented Sep 15, 2023

I'm now removing the RiffRaff deployment restriction added by @twrichards on Wednesday:

image

...I don't think I've used the deployment restriction feature before, but obviously a very good idea to add it when the latest commit on this repo was unusuable do to #184 👍

@prout-bot
Copy link

Overdue on PROD (merged by @rtyley 20 minutes and 4 seconds ago) What's gone wrong?

@rtyley
Copy link
Member Author

rtyley commented Sep 15, 2023

Well, the deploy has gone out, and PROD (https://packages.gutools.co.uk/editorial) is running (hasn't crashed like #184), so far as I can see:

image

Unfortunately though, the commit id does not come through!

% curl https://packages.gutools.co.uk/_healthcheck
Ok.
gitCommitId:Unknown

I guess GITHUB_SHA wasn't read correctly?!

@rtyley
Copy link
Member Author

rtyley commented Sep 15, 2023

I guess GITHUB_SHA wasn't read correctly?!

Oh yeah.... There is no GITHUB_SHA - because this build is currently built on TeamCity, not GitHub Actions 💥

Love it.

@Georges-GNM
Copy link
Contributor

Georges-GNM commented Sep 15, 2023

I guess GITHUB_SHA wasn't read correctly?!

Oh yeah.... There is no GITHUB_SHA - because this build is currently built on TeamCity, not GitHub Actions 💥

Love it.

Not yet anyway, but I'm not quite sure when we'll have that done for this project. Is it worth using BUILD_VCS_NUMBER (which I assume would be the correct value?) instead in the meantime?

@rtyley
Copy link
Member Author

rtyley commented Sep 15, 2023

Is it worth using BUILD_VCS_NUMBER (which I assume would be the correct value?) instead in the meantime?

I've opened #188 to cope with TeamCity for now!

@prout-bot
Copy link

Seen on PROD (merged by @rtyley 90 hours, 14 minutes and 19 seconds ago) Please check your changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants