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.

Sign up for GitHub

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

Jump to bottom

feat!: keep Item created dates when resupplying TDE-1298 #1145

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

feat!: keep Item created dates when resupplying TDE-1298 #1145

wants to merge 8 commits into from

Conversation

paulfouquet
Copy link
Collaborator

@paulfouquet paulfouquet commented Oct 24, 2024 •
edited by l0b0
Loading

Motivation

  • Ensure that the STAC Item assets.visual.created and properties.created datetimes are not changed when resupplying.
  • Ensure that the STAC Item assets.visual.updated and properties.updated datetimes all are controlled by the caller of the standardise+validate job workflow. This ensures that all the updated datetimes for the entire (re-)supply are the same.

Modifications

  • Add --current-datetime parameter to standardise+validate (RFC 3339 UTC datetime).
  • Keep created dates the same on resupply.
  • Only change asset updated datetime when its checksum changes.
  • Cache GDAL version response. This can be simplified to be an environment variable, to avoid having to shell out to GDAL.

Verification

Unit/integration/E2E tests.

@paulfouquet paulfouquet force-pushed the feat/keep-item-created-date-when-resupplying-tde-1298 branch from 2fc7ecf to f861c7b Compare October 24, 2024 20:59
@l0b0 l0b0 force-pushed the feat/keep-item-created-date-when-resupplying-tde-1298 branch 2 times, most recently from 2ec88dd to e00ccb1 Compare October 24, 2024 23:24
@paulfouquet paulfouquet force-pushed the feat/keep-item-created-date-when-resupplying-tde-1298 branch from e00ccb1 to ee8bc25 Compare October 25, 2024 00:58
@l0b0 l0b0 force-pushed the feat/keep-item-created-date-when-resupplying-tde-1298 branch 2 times, most recently from 4059d62 to 9c3dfa1 Compare October 25, 2024 02:15
@l0b0 l0b0 changed the title feat: keep Item properties.created date when resupplying feat!: keep Item properties.created date when resupplying Oct 25, 2024
@l0b0 l0b0 changed the title feat!: keep Item properties.created date when resupplying feat!: keep Item properties.created date when resupplying TDE-1298 Oct 25, 2024
@l0b0 l0b0 changed the title feat!: keep Item properties.created date when resupplying TDE-1298 feat!: keep Item created dates when resupplying TDE-1298 Oct 25, 2024
@l0b0 l0b0 force-pushed the feat/keep-item-created-date-when-resupplying-tde-1298 branch from 9c3dfa1 to d82e595 Compare October 25, 2024 02:27
@l0b0 l0b0 marked this pull request as ready for review October 25, 2024 02:32
@l0b0 l0b0 requested a review from a team as a code owner October 25, 2024 02:32
l0b0
l0b0 previously approved these changes Oct 25, 2024
View reviewed changes
@l0b0
Copy link
Contributor

l0b0 commented Oct 25, 2024

Approving as pair programmer.

@l0b0 l0b0 force-pushed the feat/keep-item-created-date-when-resupplying-tde-1298 branch from d82e595 to 8abd002 Compare October 25, 2024 02:35
l0b0
l0b0 previously approved these changes Oct 28, 2024
View reviewed changes
@paulfouquet paulfouquet force-pushed the feat/keep-item-created-date-when-resupplying-tde-1298 branch from 11f0a49 to e87afb0 Compare October 28, 2024 21:48
@l0b0 l0b0 force-pushed the feat/keep-item-created-date-when-resupplying-tde-1298 branch 5 times, most recently from 5010979 to ecd0c42 Compare October 29, 2024 03:23
@paulfouquet paulfouquet marked this pull request as draft October 29, 2024 17:55
@l0b0 l0b0 force-pushed the feat/keep-item-created-date-when-resupplying-tde-1298 branch from 6a58b41 to 42edc67 Compare October 31, 2024 01:09
@paulfouquet paulfouquet force-pushed the feat/keep-item-created-date-when-resupplying-tde-1298 branch from 42edc67 to cbda994 Compare October 31, 2024 01:54
@l0b0 l0b0 force-pushed the feat/keep-item-created-date-when-resupplying-tde-1298 branch from cbda994 to e1f1f44 Compare November 1, 2024 02:53
@l0b0 l0b0 force-pushed the feat/keep-item-created-date-when-resupplying-tde-1298 branch from e1f1f44 to 52f89ba Compare November 3, 2024 20:09
@l0b0 l0b0 marked this pull request as ready for review November 3, 2024 23:33
schmidtnz
schmidtnz reviewed Nov 4, 2024
View reviewed changes
Copy link
Contributor

@schmidtnz schmidtnz left a comment

Choose a reason for hiding this comment

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

Mostly nitpicking.
As a more general comment, it looks like harmonizing how we handle values and data for testing might potentially make our code more readable and maintainable.

scripts/stac/imagery/create_stac.py Outdated
created_datetime = updated_datetime = current_datetime

if published_path:
# FIXME: make this try/catch nicer
Copy link
Contributor

Choose a reason for hiding this comment

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

does this need to be addressed before merging?

scripts/stac/imagery/tests/generators.py Outdated
return multihash_as_hex(os.urandom(64))


def any_gdal_version() -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't appear to get used

scripts/stac/imagery/tests/generators.py Outdated
@@ -1,7 +1,11 @@
import os
Copy link
Contributor

Choose a reason for hiding this comment

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

more specific import?

@l0b0 l0b0 requested a review from schmidtnz November 5, 2024 01:21
@paulfouquet paulfouquet marked this pull request as draft November 5, 2024 19:59
l0b0
l0b0 reviewed Nov 6, 2024
View reviewed changes
scripts/stac/imagery/create_stac.py
try:
created_datetime = existing_item["properties"]["created"]
except KeyError:
get_log().info(f"Existing Item {id_} does not have 'properties.created' attribute")
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case we should not set the created datetime at all - it needs to be backfilled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@l0b0 l0b0 l0b0 left review comments

@blacha blacha Awaiting requested review from blacha blacha is a code owner automatically assigned from linz/li-topo-data-engineering

@amfage amfage Awaiting requested review from amfage amfage is a code owner automatically assigned from linz/li-topo-data-engineering

@MDavidson17 MDavidson17 Awaiting requested review from MDavidson17 MDavidson17 is a code owner automatically assigned from linz/li-topo-data-engineering

@schmidtnz schmidtnz Awaiting requested review from schmidtnz

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@paulfouquet @l0b0 @schmidtnz