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

Feature/add deployment timestamp #1100

Closed
wants to merge 2 commits into from
Closed

Conversation

GCRev
Copy link

@GCRev GCRev commented Aug 29, 2024

See: https://github.com/rstudio/rstudio-pro/issues/5597

We have this provisioning on the Workbench homepage to display the timestamp (unix epoch in seconds) for when you recently published something via rsconnect. For whatever reason, there are no timestamps added to the history entries in the history.dcf file that Workbench reads. This fixes that missing timestamp without requiring any changes on the Workbench side.

image

@GCRev GCRev added the feature a feature request or enhancement label Aug 29, 2024
@GCRev GCRev requested a review from hadley August 29, 2024 22:00
@GCRev GCRev self-assigned this Aug 29, 2024
Copy link
Contributor

@aronatkins aronatkins left a comment

Choose a reason for hiding this comment

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

Do you think this is worth mentioning in NEWS?

@@ -164,6 +166,7 @@ writeDeploymentRecord <- function(record, filePath) {
addToDeploymentHistory <- function(appPath, deploymentRecord) {
# add the appPath to the deploymentRecord
Copy link
Contributor

Choose a reason for hiding this comment

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

deploymentFields needs updating.

@@ -147,6 +148,7 @@ deploymentRecord <- function(name,
appId = appId %||% "",
bundleId = bundleId %||% "",
url = url %||% "",
when = when %||% as.numeric(as.POSIXct(Sys.time())),
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a test confirming that we read the same when data that was written.

@aronatkins
Copy link
Contributor

The when field was removed here: #770 (@hadley)

@hadley
Copy link
Member

hadley commented Aug 30, 2024

Here's the news bullet for context:

  • Deployment records no longer contain the time the app was deployed (when)
    or when it's metadata was last synched (lastSyncTime) as these variables
    are not very useful, and they lead to uninteresting diffs if you have
    committed the deployment records to git (Better handling for deleted apps #770).

IMO it's not worth adding back just for that display in workbench; if it wants that metadata it should talk to the connect API.

@GCRev
Copy link
Author

GCRev commented Aug 30, 2024

🤔 In that case I'll just remove the display field from Workbench until we work on a larger feature to obtain the history information from the connect API (which I haven't looked into yet) rather than reading a file -- if we're getting one field from an API we might as well get all of them.

The Workbench code predates any context of mine, so I assume it was probably the easiest short-term "integration" since the rworkspaces process already has access to the user's local files, so it can read this history.dcf "for free" off the file system without needing to track and manage other credentials. I suppose we already have an integration in the IDE, but I also don't know anything about that at the moment.

I'll open up a feature request for this, since I don't think it will be as trivial as a bug fix.

@GCRev GCRev closed this Aug 30, 2024
@GCRev GCRev deleted the feature/add-deployment-timestamp branch August 30, 2024 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants