Skip to content

Create REVISION_TIME with git commit timestamp upon deploy#2155

Merged
mattbrictson merged 9 commits intocapistrano:masterfrom
G-Rath:support-revision-time
Jun 20, 2024
Merged

Create REVISION_TIME with git commit timestamp upon deploy#2155
mattbrictson merged 9 commits intocapistrano:masterfrom
G-Rath:support-revision-time

Conversation

@G-Rath
Copy link
Copy Markdown
Contributor

@G-Rath G-Rath commented May 19, 2024

Summary

The general story behind this is outlined in #2153 - ultimately, this has Capistrano start to manage a REVISION_TIME file which holds a unix timestamp of when the release was committed, in the same way that it stores that releases SHA in REVISION.

Resolves #2153

Short checklist

  • Did you run bundle exec rubocop -a to fix linter issues?
  • If relevant, did you create a test?
  • Did you confirm that the RSpec tests pass?

Other Information

I think this is actually pretty close to being landable since I figured out the general "how" a while ago in another project, but I expect some tweaking will probably be required and it would be nice to support this for svn and hg if possible but I don't use those tools so will either need someone who does to provide insight or I'll see if I can google/chatgpt a solution.

Some general questions I have are:

  • do we want to have a dedicated set of tasks, or fold this into the current "revision" based ones?
  • in the context of custom SCMs, do we want to make this an optional extra or required?
  • if optional, do we need to have any custom/extra checks? (I noticed that there are some NotImplementedError methods but it didn't seem like they were actually being called by Capistrano itself)
  • if the revision time isn't available, do we want to create an REVISION_TIME file at all? (it'd be simpler, but an empty file)

@G-Rath G-Rath marked this pull request as ready for review May 20, 2024 04:30
Copy link
Copy Markdown
Member

@mattbrictson mattbrictson left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! To answer your questions:

do we want to have a dedicated set of tasks, or fold this into the current "revision" based ones?

I slightly prefer the dedicated tasks.

in the context of custom SCMs, do we want to make this an optional extra or required?

For now, I think it can be optional. I personally don't have much recent hg or svn experience, so I hesitate on making it required for those other SCMs. It looks like the only time the fetch_revision_time method is called is by the deploy:set_current_revision_time before-hook that the git plugin registers, so if that method isn't implemented by the other plugins, it shouldn't be a problem.

if the revision time isn't available, do we want to create an REVISION_TIME file at all? (it'd be simpler, but an empty file)

I think we should skip creating the REVISION_TIME file in this case (see inline comment below).

Finally, I think it would be nice to add a feature (cucumber) test for this. Maybe just a simple assertion that the REVISION_TIME file is created upon successful deploy. I'll be merging #2159 soon, which will make it easier to run the feature tests.

end
end

describe "#fetch_revision_time" do
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

✨ Thanks for the spec!

@G-Rath
Copy link
Copy Markdown
Contributor Author

G-Rath commented Jun 16, 2024

@mattbrictson thanks for the review! I'm assuming based on the comments (or lack thereof) on the PR that my changes already match the opinions you've expressed in reply to my questions, but let me know if that's not the case!

In the meantime I'll checkout #2159 and see if I can get a feature test crafted

@G-Rath

This comment was marked as resolved.

@G-Rath G-Rath requested a review from mattbrictson June 17, 2024 19:51
Then(/^the REVISION file is created in the release$/) do
stdout, _stderr = run_remote_ssh_command("cat #{@release_paths[0]}/REVISION")

# TODO: confirm that this value is actually constant for everyone
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

give the specs are passing, it seems like these are consistent though I'm still not sure where they're coming from exactly - either way, I'll leave these TODOs in at least until #2159 is landed

@mattbrictson
Copy link
Copy Markdown
Member

it's doing ls -g of the releases path rather than an actual release, so if I understand correctly it will only error if the releases directory doesn't exist - is that intentional?

@G-Rath I think you're right. Do you mind opening a GitHub issue for this so we can track it as a bug?

@G-Rath

This comment was marked as resolved.

@mattbrictson mattbrictson changed the title feat: support REVISION_TIME Create REVISION_TIME with git commit timestamp upon deploy Jun 20, 2024
@mattbrictson mattbrictson added the ✨ Feature Adds a new feature label Jun 20, 2024
@mattbrictson mattbrictson merged commit c30610d into capistrano:master Jun 20, 2024
@G-Rath G-Rath deleted the support-revision-time branch June 21, 2024 00:14
@mattbrictson
Copy link
Copy Markdown
Member

@G-Rath if for whatever reason, someone wanted to disable this new REVISION_TIME file from being created, how would they do that? I want to document it in the release notes, just to be safe.

@G-Rath
Copy link
Copy Markdown
Contributor Author

G-Rath commented Jun 24, 2024

@mattbrictson the sure fire way would be to ensure that fetch(:current_revision_time) returns a false-y value, which could be done by registering a task straight after the deploy:set_current_revision_time task that explicitly sets it to nil.

I've not done it myself but I assume they could also remove or override one of the tasks in the chain, such as deploy:set_previous_revision_time

@G-Rath
Copy link
Copy Markdown
Contributor Author

G-Rath commented Jun 24, 2024

Again, not done it myself but this is what I was thinking of - that should be enough right? combined with the task name of deploy:set_current_revision_time

@mattbrictson
Copy link
Copy Markdown
Member

@G-Rath yes, that makes sense. I'll test that out.

@mattbrictson
Copy link
Copy Markdown
Member

This seems to do the trick:

Rake::Task["git:set_current_revision_time"].clear_actions

@mattbrictson
Copy link
Copy Markdown
Member

This feature has been released in Capistrano 3.19.0: https://github.com/capistrano/capistrano/releases/tag/v3.19.0

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

Labels

✨ Feature Adds a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support exposing revision time alongside revision sha (when using git)

3 participants