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

Update Project parser to support commit timestamps #6517

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

grahamplata
Copy link
Contributor

@grahamplata grahamplata commented Jan 28, 2025

This PR updates the project parser to support commit timestamps. Enabling the UI to display the last timestamp the source project files were updated.

Git-connected projects, will utilize the current commit's timestamp, while non-Git deploys that will be the timestamp when the project files being served were uploaded.

@grahamplata grahamplata changed the title Update Project parser to support commit time stamps Update Project parser to support commit timestamps Jan 28, 2025
@grahamplata grahamplata marked this pull request as ready for review January 28, 2025 15:54
proto/rill/runtime/v1/resources.proto Outdated Show resolved Hide resolved
Comment on lines 38 to 42
if h.downloadURL != "" {
// use downloadURL as a proxy for CommitTimestamp for one-time uploads
// It will change when new data is uploaded
return time.Now(), nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. In this case (where there is no connected Git repository), it needs to return the time the current project files were uploaded. You'll need to propagate that timestamp along with downloadURL – if you trace the provenance of downloadURL, you'll find you can access that timestamp as asset.CreatedOn here:
    return &adminv1.GetRepoMetaResponse{
  2. Looking in the old CommitHash implementation below, it returns h.downloadURL as the hash in the same case. That is actually not safe, and it would be good if you could fix that in this PR as well now that we're at it. Tracing it back as described above, it would be better to return that field as the value of asset.ID (which only changes if there's a new upload and does not leak the ability to download the files).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@begelundmuller that makes sense to me. Made some adjustments

runtime/reconcilers/project_parser.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants