Expose timestamp and checksum to status #216

stephenprater · 2019-06-17T22:30:47Z

What changes does this PR introduce?

This introduces two special variables {{.CHECKSUM}} and {{.TIMESTAMP}} to status commands, to enable the checking of "up-to-dateness" of remote artifacts.

Any background context you want to provide?

Generally, when you use sources - you also use generates - if the generates artifacts is newer than the sources, then the your task is out of date and needs to be re-run. This works as long as you have direct access to the generated artifacts - but some tasks produce "remote artifacts" that don't live on the same file system as your sources (AMIs, Docker Images, Kubernetes Deploys) - status already supports sh based status checks - so we expanded this to evaluate remote artifacts.

Which ever method you choose to fingerprint your sources will be exposed as a template variable to status shell commands.

Currently, the expectation is that you would use sources and generates together and then status separately. The expectation going forward would be that sources could be used with either status or generates to evaluate the status of the task. status and generates are still mutually exclusive.

Where should the reviewer start?

The test is a little artificial in that it echos the value of the checksum rather than actually doing something with it - but it demonstrates how the feature could be used.

Has this been manually tested? How?

This is part of our private fork.

andreynering

Hi @stephenprater,

Pretty good work, thanks for opening this pull request. 🙂

I did a couple of comments. The most challenging one is the suggestion to change .TIMESTAMP from a string to a plain time.Time struct, which may require changes on how Task handles variables (which are just strings currently) to accept interface{}s.

Don't hesitate to ask if you need help or guidance help.

andreynering · 2019-06-18T00:25:26Z

docs/usage.md

 - test -f directory/file2.txt
 ```

+Normally, you would use either `status` or `sources` in combination with


Since the "Prevent unnecessary work" section is becaming big, perhaps we could separate it into subsections (using ### for subtitles) for sources/generates vs. status vs. preconditions.

What do you think?

docs/usage.md

andreynering · 2019-06-18T00:26:54Z

internal/status/checksum.go

 return fmt.Sprintf("%x", h.Sum(nil)), nil
 }

+// Value implements the Chcker Interface


Suggested change

// Value implements the Chcker Interface

// Value implements the Checker interface

Also, missing documentation comments for Kind() functions.

variables.go

andreynering · 2019-06-18T00:35:20Z

variables.go


+if len(origTask.Status) > 0 {
+
+e := &Executor{


Can't we use the existing Executor instance instead of instantiating another one?

andreynering · 2019-06-18T00:40:18Z

variables.go

+return nil, err
+}
+
+vars[strings.ToUpper(checker.Kind())] = taskfile.Var{Static: value}


I think we should this whole if block to the bottom of the function. Otherwise the TIMESTAMP and CHECKSUM variables will be available to the stuff parsed below, but not to those parse above, which would be a bit confusing. 🙂

Alternatively, we could clone the vars map to prevent that.

andreynering · 2019-06-18T00:41:28Z

variables.go

+
+vars[strings.ToUpper(checker.Kind())] = taskfile.Var{Static: value}
+
+statusTemplater := templater.Templater{Vars: vars}


Unless the clone vars, we don't need to instantiate another templater since maps are pass-by-reference.

andreynering · 2019-06-18T00:42:52Z

status.go

 }

-func (e *Executor) getStatusChecker(t *taskfile.Task) (status.Checker, error) {
+func (e *Executor) GetStatusChecker(t *taskfile.Task) (status.Checker, error) {


You don't necessarily need to make this function exported, since it's only called inside this package anyway.

andreynering · 2019-06-18T00:55:52Z

internal/status/timestamp.go

+return "0", nil
+}
+
+return fmt.Sprintf("%d", sourcesMaxTime.Unix()), nil


Hmm... something to discuss here:

We could possibly pass the time.Time struct to the template instead of converting it to string.

In that case, users would have to use {{.TIMESTAMP | unix}} to get the user timestamp, or {{.TIMESTAMP | "2006-01-02"}} to format it to the format they want to use.

This would need to be documented, and the return of Value() would be changed to (interface{}, error).

What do you think?

I am 100% on board with that.

[ "{{.TIMESTAMP}}" -gt "$({{.DATE_CMD}} --date $last_ami_date +"%s")" ]

Here's part of my actual invocation - where I have to parse a Date string from AWS into a unix timestamp while also templating in a system dependent invocation of date. Yeah - I'd rather do that in go templates for sure.

stephenprater

All great feedback. I'm definitely down for changing the type signature of Value.

stephenprater · 2019-06-18T01:54:56Z

internal/status/timestamp.go

+return "0", nil
+}
+
+return fmt.Sprintf("%d", sourcesMaxTime.Unix()), nil


I am 100% on board with that.

[ "{{.TIMESTAMP}}" -gt "$({{.DATE_CMD}} --date $last_ami_date +"%s")" ]

Here's part of my actual invocation - where I have to parse a Date string from AWS into a unix timestamp while also templating in a system dependent invocation of date. Yeah - I'd rather do that in go templates for sure.

andreynering · 2019-08-12T01:40:35Z

Hi @stephenprater,

Sorry for asking, but just to make sure we're on the same page... Do you plan to do the necessary fixes or do you prefer me to work on it?

If the latter, can you make sure this checkbox is checked?

Thanks!

stephenprater · 2019-08-12T02:16:09Z

Sorry Andrey, I’ve been kind of distracted the last week or so. I think I should be able to get to this tomorrow evening.

…

On Aug 11, 2019, 6:40 PM -0700, Andrey Nering ***@***.***>, wrote: Hi @stephenprater, Sorry for asking, but just to make sure we're on the same page... Do you plan to do the necessary fixes or do you prefer me to work on it? If the latter, can you make sure this checkbox is checked? Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

andreynering · 2019-08-12T02:23:03Z

@stephenprater Don't hurry, take your time, we all have busy lives. I was just checking in. 😉

Thanks!

andreynering · 2019-08-12T03:06:06Z

Can you please move the target branch from master to v3 and merge v3 into this branch? We shouldn't have any conflicts. Thanks!

Co-Authored-By: Andrey Nering <andrey.nering@gmail.com>

…tatus * upstream/v3: v3.0.0-preview1 Update v3 changelog Only have colored output on v3 Add --color=false flag to disable colored output Update documentation about sprig Update CHANGELOG Migrate from sprig to slim-sprig Fix build after merging master Use colors for some output messages

stephenprater · 2019-08-25T20:29:55Z

@andreynering Sorry for the delay - the company I was doing this work for is doing some restructuring (notably, restructuring me and most of the employees out of a job). I made the changes you requested in terms of code review and took a stab at making the Checker#Value method use the empty interface.

I had some trouble getting this to work without adding an additional "kind" of variable. Since the "static" variables are recursively replaced by their own values, I couldn't figure out how to allow the Replace function to accept both a "template string" and an empty interface as the value. My solution was to add another "Variable Kind" - Live - which is basically a real Go value exposed to the templating system, that never recursively replaces itself.

The strMap cache in the templater needed it's typing relaxed as well, since it's no longer strictly a string -> string map, but could be string -> (pretty much anything).

* master: Update CHANGELOG Small improvements to go-task#228 Fix a typo Fix Checksum.IsUpToDate Remove directory check Update glob.go Separate error handlings for readability Re-run the task if generated files do not exist

andreynering · 2019-08-25T21:03:43Z

Hi @stephenprater,

I'm really sorry to know that. All the luck on the search for a new job!

Thanks for the updates, I'll review again once possible.

…rpunkArmory/task into CypherpunkArmory-report-timestamp-to-status

andreynering · 2019-09-14T21:14:49Z

Hi @stephenprater,

This is now merged on the v3 branch.

I did a few improvements here and here.

Thanks a lot for the contribution and patience. It's much appreciated! ❤️

andreynering reviewed Jun 18, 2019

View reviewed changes

stephenprater commented Jun 18, 2019

View reviewed changes

andreynering added the feature label Aug 12, 2019

stephenprater and others added 4 commits August 25, 2019 09:39

Expose timestamp and checksum to status

1ee684b

Apply suggestions from code review

e0f72a6

Co-Authored-By: Andrey Nering <andrey.nering@gmail.com>

Changes per feedback

db36bc6

stephenprater force-pushed the report-timestamp-to-status branch from 3951956 to 9aa9033 Compare August 25, 2019 20:17

stephenprater added 3 commits August 25, 2019 13:36

Export Time Struct to Template

a1aec81

Merge branch 'master' into report-timestamp-to-status

d1183ce

* master: Update CHANGELOG Small improvements to go-task#228 Fix a typo Fix Checksum.IsUpToDate Remove directory check Update glob.go Separate error handlings for readability Re-run the task if generated files do not exist

Fix tests

6b0935d

stephenprater force-pushed the report-timestamp-to-status branch from 9aa9033 to 6b0935d Compare August 25, 2019 20:47

andreynering changed the base branch from master to v3 August 25, 2019 21:04

Merge branch 'report-timestamp-to-status' of https://github.com/Cyphe…

1a33f91

…rpunkArmory/task into CypherpunkArmory-report-timestamp-to-status

andreynering changed the base branch from v3 to master September 2, 2019 00:51

andreynering changed the base branch from master to v3 September 2, 2019 00:51

andreynering added this to the v3 milestone Sep 2, 2019

andreynering added the v3 label Sep 2, 2019

andreynering added a commit that referenced this pull request Sep 14, 2019

Few code improvements on #216

1a28e5e

andreynering added a commit that referenced this pull request Sep 14, 2019

Add CHANGELOG for #216

d3060b0

andreynering merged commit 1a33f91 into go-task:v3 Sep 14, 2019

andreynering mentioned this pull request Sep 14, 2019

Roadmap to version 3 #195

Closed

18 tasks

pd93 mentioned this pull request Mar 22, 2023

Special CHECKSUM variable only ever yields d41d8cd98f00b204e9800998ecf8427e #1076

Closed

pd93 mentioned this pull request Mar 22, 2023

fix: checksum variable in status is incorrect #1080

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Expose timestamp and checksum to status #216

Expose timestamp and checksum to status #216

Uh oh!

stephenprater commented Jun 17, 2019

andreynering left a comment

andreynering Jun 18, 2019

Uh oh!

andreynering Jun 18, 2019

andreynering Jun 18, 2019

Uh oh!

andreynering Jun 18, 2019

andreynering Jun 18, 2019

andreynering Jun 18, 2019

andreynering Jun 18, 2019

andreynering Jun 18, 2019

stephenprater Jun 18, 2019

stephenprater left a comment

stephenprater Jun 18, 2019

andreynering commented Aug 12, 2019

stephenprater commented Aug 12, 2019 via email

andreynering commented Aug 12, 2019

andreynering commented Aug 12, 2019

stephenprater commented Aug 25, 2019 •

edited

Loading

andreynering commented Aug 25, 2019

andreynering commented Sep 14, 2019

Labels

2 participants

	// Value implements the Chcker Interface
	// Value implements the Checker interface


		vars[strings.ToUpper(checker.Kind())] = taskfile.Var{Static: value}

		statusTemplater := templater.Templater{Vars: vars}

Uh oh!

Expose timestamp and checksum to status #216

Expose timestamp and checksum to status #216

Uh oh!

Conversation

stephenprater commented Jun 17, 2019

What changes does this PR introduce?

Any background context you want to provide?

Where should the reviewer start?

Has this been manually tested? How?

andreynering left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenprater left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreynering commented Aug 12, 2019

stephenprater commented Aug 12, 2019 via email

andreynering commented Aug 12, 2019

andreynering commented Aug 12, 2019

stephenprater commented Aug 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

andreynering commented Aug 25, 2019

andreynering commented Sep 14, 2019

Labels

2 participants

stephenprater commented Aug 25, 2019 •

edited

Loading