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

promote PLUTO along with ZTL if specified, use dev buckets #1176

Merged
merged 8 commits into from
Oct 9, 2024

Conversation

fvankrieken
Copy link
Contributor

@fvankrieken fvankrieken commented Oct 4, 2024

Lot of testing for a pretty simple feature! Ended up spending most of time working on kinks around my data setup in the dev bucket. Overall changes are simple enough to follow! Plus I fix some things in sequential commits, so just look at overall

Two main things

  • when we both build pluto as part of ztl dataloading action and promote ztl to draft in the same action, also promote pluto
  • make use of dev buckets an option for ztl dataloading, promote to draft, and publish actions

Ztl dataloading action runs

Dev buckets working for dataloading
image

Automated promotion to draft
image

Job promoting to publish in dev bucket here. Important note that I added logic to not create a github tag for dev runs of this. Below, 2024-10-01 was promoted from draft folder in dev bucket, not copied from edm-publsihing.
image

@fvankrieken
Copy link
Contributor Author

Oops sorry - need to let some data finish being loaded to my dev bucket before I can rerun the the ztl dataloading job one more time

@fvankrieken fvankrieken marked this pull request as draft October 8, 2024 13:51
@fvankrieken fvankrieken marked this pull request as ready for review October 8, 2024 19:27
@sf-dcp
Copy link
Contributor

sf-dcp commented Oct 8, 2024

I think there is an issue with the logging table when you run this code (found the issue by accident while investing the logging table)...

This is the latest run I see for today.

And here is the logging table for ztl (looks same for PLUTO):

image

It seems like build events with dev flag don't get logged while promote_to_draft do (not expected behavior). Not sure whether logging happens with the publish event, with dev flag set to true.

I don't think we want to log dev stuff; unless we indicate a presence of dev flag in the custom_fields column

@fvankrieken
Copy link
Contributor Author

I think there is an issue with the logging table when you run this code (found the issue by accident while investing the logging table)...

This is the latest run I see for today.

And here is the logging table for ztl (looks same for PLUTO):

image

It seems like build events with dev flag don't get logged while promote_to_draft do (not expected behavior). Not sure whether logging happens with the publish event, with dev flag set to true.

I don't think we want to log dev stuff; unless we indicate a presence of dev flag in the custom_fields column

Ooh good catch. I'll look into that

@fvankrieken
Copy link
Contributor Author

fvankrieken commented Oct 9, 2024

Do we want to be logging db-template events? I would lean no

Also, I deleted the rows I generated through this in the logging table

fixed publish action here

@fvankrieken fvankrieken requested a review from sf-dcp October 9, 2024 14:13
Copy link

codecov bot commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.87%. Comparing base (f69651e) to head (8683822).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1176      +/-   ##
==========================================
+ Coverage   67.80%   67.87%   +0.06%     
==========================================
  Files         109      109              
  Lines        5746     5746              
  Branches      846      846              
==========================================
+ Hits         3896     3900       +4     
+ Misses       1721     1718       -3     
+ Partials      129      128       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -69,6 +69,7 @@ jobs:
image: nycplanning/build-base:latest
env:
PUBLISHING_BUCKET: ${{ inputs.dev_bucket && format('de-dev-{0}', inputs.dev_bucket) || 'edm-publishing' }}
DEV_FLAG: ${{ inputs.dev_bucket && 'true' || 'false' }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Long term this doesn't feel the most sustainable way of handling this, ideally we have a single value somewhere setting the environment and we have a script or something handle translating that to buckets, db connection strings (if we had different db envs), etc... but this is what we currently have in other places and I think it's simplest for now

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we could also add a condition, not necessarily here, to only run a job if one of the following conditions is true?

(DEV_FLAG = false and dev_bucket is an empty string)
or
(DEV_FLAG = true and dev_bucket is not empty)

Copy link
Contributor

Choose a reason for hiding this comment

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

You are already addressing the second condition in the DEV_FLAG

Copy link
Contributor

Choose a reason for hiding this comment

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

or maybe I'm tripping and we don't need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would lean with no - it's just adding another thing that needs to be manually specified. Right now, the dev bucket is really the only flag we have, and I think it makes sense to set a dev flag this way. And I'd rather put effort into consolidating logic to generate the various variables related to some sort of overall env flag than to try to add logic to validate that they're consistent

@sf-dcp
Copy link
Contributor

sf-dcp commented Oct 9, 2024

Do we want to be logging db-template events? I would lean no

Also, I deleted the rows I generated through this in the logging table

fixed publish action here

I think we originally decided to log db-template events. I'm personally indifferent. Perhaps I'm leaning towards not logging them because every triggered test logs db-template, resulting in many records in the logging table

@@ -55,7 +59,8 @@ jobs:
container:
image: nycplanning/build-base:latest
env:
PUBLISHING_BUCKET: edm-publishing
PUBLISHING_BUCKET: ${{ inputs.dev_bucket && format('de-dev-{0}', inputs.dev_bucket) || 'edm-publishing' }}
DEV_FLAG: ${{ inputs.dev_bucket && 'true' || 'false' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as for promote_to_draft.yml

@@ -121,7 +121,7 @@ def label(self) -> str:
if self.patch == 0:
return self.date.strftime("%Y-%m-%d")
else:
return f"{self.date.strftime('%Y-%m')}.{self.patch}"
return f"{self.date.strftime('%Y-%m-%d')}.{self.patch}"
Copy link
Contributor

Choose a reason for hiding this comment

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

uh oh!

Copy link
Contributor

@sf-dcp sf-dcp left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for fixing a silly error with patching.

I tripped on GH syntax around setting up a dev flag. I thought dev flag is a user input variable in addition to the bucket

@fvankrieken fvankrieken merged commit 3f3eb1a into main Oct 9, 2024
21 checks passed
@fvankrieken fvankrieken deleted the fvk-patch branch October 9, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants