Add support for timestamped split registry endpoint #97

reggieag · 2020-01-31T20:51:19Z

/domain @Betterment/test_track_core
/platform @aburgel @jmileham @effron @smudge

This PR adds the build timestamp to the rails client. We decided to push the timestamp generation and validation up to the build of the client so the app code can assume that the timestamp exists.

nanda-prbot · 2020-01-31T20:51:23Z

Needs somebody from @Betterment/test_track_core to claim domain review
Needs somebody from @aburgel, @jmileham, @effron, and @smudge to claim platform review

Use the shovel operator to claim, e.g.:

@myname << domain && platform

_{HOW TO: Claim a Review}

aburgel · 2020-02-01T21:42:50Z

<< platform

nanda-prbot · 2020-02-01T21:42:56Z

Needs somebody from @Betterment/test_track_core to claim domain review

Use the shovel operator to claim, e.g.:

@myname << domain && platform

_{HOW TO: Claim a Review}

aburgel · 2020-02-01T21:28:35Z

app/models/test_track/remote/split_registry.rb

@@ -15,7 +15,7 @@ def instance
      if faked?
        new(fake_instance_attributes(nil))
      else
-        get('/api/v2/split_registry')
+        get("/api/v3/builds/#{TestTrack::BuildTimestamp}/split_registry")


We may be able to pass in the timestamp as an argument to get. Though we don’t need url encoding since the timestamp shouldn’t have any funky characters, I think it’s a good practice.

Seems like we're doing string interpolation in the other instances of get with args. Should we stick with that pattern or forge our own path?

app/models/test_track/remote/split_registry.rb

aburgel · 2020-02-01T21:33:28Z

config/initializers/test_track_generate_timestamp.rb

+elsif File.exist?(BUILD_TIMESTAMP_FILE_PATH) && File.readable?(BUILD_TIMESTAMP_FILE_PATH) && !File.zero?(BUILD_TIMESTAMP_FILE_PATH)
+  TestTrack::BuildTimestamp = File.read(BUILD_TIMESTAMP_FILE_PATH)
+else
+  raise Exception.new "Failed to read timestamp"


This could use more context. If the app fails to startup we should provide plenty of details on why and how to fix.

Something like: “TestTrack failed to load the required build timestamp. Ensure testtrack-cli build-timestamp is run and the timestamp file is present”

Something like that but better 😁

Any reason we're raising a bare exception instead of just the text, which would result in a StandardError?

aburgel · 2020-02-01T21:35:02Z

config/initializers/test_track_generate_timestamp.rb

+if Rails.env.test? || Rails.env.development?
+  TestTrack::BuildTimestamp = Time.zone.now.iso8601
+elsif File.exist?(BUILD_TIMESTAMP_FILE_PATH) && File.readable?(BUILD_TIMESTAMP_FILE_PATH) && !File.zero?(BUILD_TIMESTAMP_FILE_PATH)
+  TestTrack::BuildTimestamp = File.read(BUILD_TIMESTAMP_FILE_PATH)


I think some basic validation of the file content is worthwhile, like is it in iso format. @jmileham does the iOS client do any validation?

I'm not sure, but agreed. Also, this camelization isn't conventional for ruby - it looks like a class. We should either have a property of TestTrack, a la TestTrack.build_timestamp, or it should be using constant case, i.e. TestTrack::BUILD_TIMESTAMP. If we don't intend for it to be mutable, we should do that. Also, the timestamp file should probably be newline terminated, and if so, this should probably be chomping the results during its validation/normalization phase.

aburgel · 2020-02-01T21:37:27Z

lib/tasks/test_track_rails_client_tasks.rake

+    next if cli.skip_testtrack_cli?
+
+    if cli.project_initialized?
+      result = cli.call('generate_timestamp')


Hmm, the naming of this command and the rake task is inconsistent. I like the more explicit name of the rake task, so probably should change the name of this command to generate_build_timestamp.

so update the command in the cli?

aburgel · 2020-02-01T21:39:15Z

lib/tasks/test_track_rails_client_tasks.rake

@@ -31,3 +42,4 @@ end
 task 'db:schema:load' => ['test_track:schema:load']
 task 'db:structure:load' => ['test_track:schema:load']
 task 'db:migrate' => ['test_track:migrate']
+task 'deploy:assets:precompile' => ['test_track:generate_build_timestamp']


Would be nice to add a test for this since it’s now pretty critical to building the apps, not sure if we have any examples of testing rake tasks.

aburgel · 2020-02-01T21:42:15Z

config/initializers/test_track_generate_timestamp.rb

@@ -0,0 +1,9 @@
+BUILD_TIMESTAMP_FILE_PATH = 'testtrack/build_timestamp'.freeze
+
+if Rails.env.test? || Rails.env.development?


Getting some tests around this behavior would be great too. Not sure if you can test an initializer. But we could pull this logic into a function, test that and then just call that here

aburgel · 2020-02-01T21:43:46Z

Tafn! This is great. Mostly comments about test coverage. Nice work!

nanda-prbot · 2020-02-01T21:43:51Z

Needs somebody from @Betterment/test_track_core to claim domain review

Use the shovel operator to claim, e.g.:

@myname << domain && platform

_{HOW TO: Claim a Review}

* constant name case * cli task name to match updated go cli * update cli version

removes from the initializer to write tests. Also relies on some assumptions to simplify the conditional logic.

* renames initialzier method to load_build_timestamp * vendorizes new version of cli * adds more error handling around the timestamp loading and reading * uses corerct assets precompile naming

phantomphildius · 2020-02-14T22:30:10Z

nanda

nanda-prbot · 2020-02-14T22:30:16Z

Needs somebody from @Betterment/test_track_core to claim domain review

Use the shovel operator to claim, e.g.:

@myname << domain && platform

_{HOW TO: Claim a Review}

aburgel · 2020-02-19T01:51:32Z

spec/test_track_spec.rb

+    around { |example| Timecop.freeze(Time.zone.parse('2020-02-01')) { example.run } }
+
+    context 'in a test environment' do
+      it 'assigns BUILD_TIMESTAMP to now' do


Suggested change

it 'assigns BUILD_TIMESTAMP to now' do

it 'assigns build_timestamp to now' do

aburgel · 2020-02-19T01:51:41Z

spec/test_track_spec.rb

+    end
+
+    context 'in a development environment' do
+      it 'assigns BUILD_TIMESTAMP to now' do


Suggested change

it 'assigns BUILD_TIMESTAMP to now' do

it 'assigns build_timestamp to now' do

aburgel · 2020-02-19T01:52:10Z

spec/test_track_spec.rb

+      end
+
+      context 'with an existing build_timestamp file' do
+        it 'assigns BUILD_TIMESTAMP to file\'s contents' do


Suggested change

it 'assigns BUILD_TIMESTAMP to file\'s contents' do

it 'assigns build_timestamp to file\'s contents' do

aburgel · 2020-02-19T01:53:44Z

spec/test_track_spec.rb

+
+      before do
+        allow(File).to receive(:exist?).and_return(file_readable)
+        allow(File).to receive(:read).and_return("2020-02-01T00:00:00Z\n")


since you're freezing time on line 191 to the exact same time as what you're stubbed here, its a hard to be confident that we're actually reading from the file and not from frozen time

aburgel · 2020-02-19T01:57:01Z

lib/test_track.rb

+      elsif File.exist?(BUILD_TIMESTAMP_FILE_PATH)
+        raise "TestTrack's build_timestamp is not formatted properly." unless BUILD_TIMESTAMP_REGEX.match?(_build_timestamp)
+
+        @build_timestamp = _build_timestamp


very minor nit. you're reading the file twice. on this line and line 61, you could capture build_timestamp in a local var to avoid that

aburgel · 2020-02-19T02:08:05Z

config/initializers/test_track_load_timestamp.rb

@@ -0,0 +1 @@
+TestTrack.load_build_timestamp


do we need test_track in the filename? may just load_build_timestamp? since test_track is kinda implied

aburgel · 2020-02-19T02:13:07Z

lib/test_track.rb

+        @build_timestamp = _build_timestamp
+      else
+        raise 'TestTrack failed to load the required build timestamp. ' \
+          'Ensure `testtrack load_build_timestamp` is run and the build timestamp file is present.'


Suggested change

'Ensure `testtrack load_build_timestamp` is run and the build timestamp file is present.'

'Ensure `test_track:generate_build_timestamp` task is run and the build timestamp file is present.'

aburgel · 2020-02-19T02:14:27Z

lib/test_track.rb

+      if Rails.env.test? || Rails.env.development?
+        @build_timestamp = Time.zone.now.iso8601
+      elsif File.exist?(BUILD_TIMESTAMP_FILE_PATH)
+        raise "TestTrack's build_timestamp is not formatted properly." unless BUILD_TIMESTAMP_REGEX.match?(_build_timestamp)


Suggested change

raise "TestTrack's build_timestamp is not formatted properly." unless BUILD_TIMESTAMP_REGEX.match?(_build_timestamp)

raise "./testtrack/build_timestamp is not a valid ISO 8601 timestamp, got '#{_build_timestamp}'" unless BUILD_TIMESTAMP_REGEX.match?(_build_timestamp)

aburgel · 2020-02-19T02:17:47Z

lib/test_track.rb

+    end
+
+    def build_timestamp
+      @build_timestamp.presence || raise('build_timestamp is not defined. Ensure `load_build_timestamp` is run.')


Suggested change

@build_timestamp.presence || raise('build_timestamp is not defined. Ensure `load_build_timestamp` is run.')

@build_timestamp.presence || raise('build_timestamp is not defined because `load_build_timestamp` initializer was not run.')

aburgel · 2020-02-19T02:20:56Z

platformlgtm! looking great! dropped in a couple of minor suggestions around the exception messages.

nanda-prbot · 2020-02-19T02:21:05Z

Needs somebody from @Betterment/test_track_core to claim domain review

Use the shovel operator to claim, e.g.:

@myname << domain && platform

_{HOW TO: Claim a Review}

phantomphildius · 2020-02-21T20:25:16Z

nanda

nanda-prbot · 2020-02-21T20:25:24Z

Needs somebody from @Betterment/test_track_core to claim domain review

Use the shovel operator to claim, e.g.:

@myname << domain && platform

_{HOW TO: Claim a Review}

smudge · 2020-02-22T00:02:53Z

lib/test_track.rb

+    end
+
+    def build_timestamp
+      @build_timestamp.presence || raise('build_timestamp is not defined because `load_build_timestamp` initializer was not run.')


I wonder if this should note that it normally runs as part of assets:precompile

(same comment for the error above)

smudge · 2020-02-22T00:15:40Z

lib/test_track.rb

+    def load_build_timestamp # rubocop:disable Metrics/MethodLength
+      if Rails.env.test? || Rails.env.development?
+        @build_timestamp = Time.zone.now.iso8601
+      elsif File.exist?(BUILD_TIMESTAMP_FILE_PATH)


I'm thinking we can maybe combine the error cases so we only have to raise once (regardless of whether the file exists)

def load_build_timestamp if Rails.env.test? || Rails.env.development? @build_timestamp = Time.zone.now.iso8601 elsif _build_timestamp unless BUILD_TIMESTAMP_REGEX.match?(timestamp) raise "./testtrack/build_timestamp is not a valid ISO 8601 timestamp, got '#{timestamp}'" end @build_timestamp = _build_timestamp else raise '...' end end attr_reader :build_timestamp # ... def _build_timestamp File.read(BUILD_TIMESTAMP_FILE_PATH).chomp.presence if File.exist?(BUILD_TIMESTAMP_FILE_PATH) end

Main thing there is that by calling .presence sooner and sharing the raise during the initializer code, we can make build_timestamp an attr_reader and read straight off the ivar.

(I'm thinking this is a safe strategy because the gem itself defines the initializer and owns load_build_timestamp, so unless we suspect that somehow folks are gonna skip initialization of the app?)

(Alternatively, I sort of wonder if we could rename load_build_timestamp to build_timestamp -- have it fetch all the things internally, run all the checks, etc -- and then basically just pre-heat it from the initializer.)

smudge · 2020-02-22T00:43:55Z

<< domain LGTM - this looks totally correct to me. I don't want to block you from merging, but I did leave a pile o' structural suggestions, take them or leave them. (Also I'm thinking we could mention the assets:precompile behavior in the error messages.)

nanda-prbot · 2020-02-22T00:44:03Z

Approved! 🌮 💥 💰

phantomphildius · 2020-03-05T21:15:14Z

lib/tasks/test_track_rails_client_tasks.rake

@@ -31,3 +39,4 @@ end
 task 'db:schema:load' => ['test_track:schema:load']
 task 'db:structure:load' => ['test_track:schema:load']
 task 'db:migrate' => ['test_track:migrate']
+task 'assets:precompile' => ['test_track:generate_build_timestamp']


going to try reordering the tasks and adding a flag to not run the initializers until this is done here

phantomphildius · 2020-04-11T15:22:07Z

nanda

nanda-prbot · 2020-04-11T15:22:12Z

Approved! ✨ 💥 🎯

aburgel · 2020-04-13T14:26:39Z

config/initializers/load_build_timestamp.rb

@@ -0,0 +1 @@
+TestTrack.load_build_timestamp unless ENV['SKIP_TIMESTAMP_INIT'] == '1'


Suggested change

TestTrack.load_build_timestamp unless ENV['SKIP_TIMESTAMP_INIT'] == '1'

TestTrack.load_build_timestamp unless ENV['SKIP_TESTTRACK_LOAD_BUILD_TIMESTAMP'] == '1'

aburgel · 2020-04-13T14:28:14Z

lib/tasks/test_track_rails_client_tasks.rake

@@ -10,6 +10,19 @@ namespace :test_track do
    end
  end

+  desc 'Generates build timestamp for consuming applications to build versioned requests to split registry'


Suggested change

desc 'Generates build timestamp for consuming applications to build versioned requests to split registry'

desc 'Generates build timestamp for fetching point-in-time split registries'

lib/tasks/test_track_rails_client_tasks.rake

reggieag and others added 3 commits January 24, 2020 16:03

adding timestamp

4b54967

passing tests

4faffec

adding binaries

2471575

nanda-prbot added domain-not-claimed platform-not-claimed labels Jan 31, 2020

nanda-prbot removed the platform-not-claimed label Feb 1, 2020

aburgel reviewed Feb 1, 2020

View reviewed changes

phantomphildius added 4 commits February 7, 2020 16:44

Update naming and versions

326c595

* constant name case * cli task name to match updated go cli * update cli version

extract timestamp reading to TestTrack module

32d919e

removes from the initializer to write tests. Also relies on some assumptions to simplify the conditional logic.

rough rake task spec

8b44bc8

live code review with burgel

ea9f01b

* renames initialzier method to load_build_timestamp * vendorizes new version of cli * adds more error handling around the timestamp loading and reading * uses corerct assets precompile naming

aburgel reviewed Feb 19, 2020

View reviewed changes

renaming and rewording

f84e455

smudge reviewed Feb 22, 2020

View reviewed changes

nanda-prbot added approved and removed domain-not-claimed labels Feb 22, 2020

phantomphildius reviewed Mar 5, 2020

View reviewed changes

phantomphildius and others added 5 commits March 6, 2020 15:03

hack rake task to only run initializer once date is generated

63b54c6

try a different method to enhance the environment task

f376d3f

another one

a81e6f7

Update testtrack-cli to 1.1.1

899db38

clean up and error messages

1b2c855

aburgel reviewed Apr 13, 2020

View reviewed changes

phantomphildius added 3 commits April 14, 2020 12:32

call presence earlier

f68b3f8

warm the cache

ff8cc0a

set timstamp in env vars too

991d911

aburgel changed the title ~~Add build timestamp to rails client~~ Add support for timestamped split registry endpoint Apr 15, 2020

aburgel merged commit e0b7039 into Betterment:master Apr 15, 2020

phantomphildius deleted the rg/add-timestamp branch April 24, 2023 20:34

		@@ -0,0 +1,9 @@
		BUILD_TIMESTAMP_FILE_PATH = 'testtrack/build_timestamp'.freeze

		if Rails.env.test? \|\| Rails.env.development?

	it 'assigns BUILD_TIMESTAMP to now' do
	it 'assigns build_timestamp to now' do

	it 'assigns BUILD_TIMESTAMP to file\'s contents' do
	it 'assigns build_timestamp to file\'s contents' do

	'Ensure `testtrack load_build_timestamp` is run and the build timestamp file is present.'
	'Ensure `test_track:generate_build_timestamp` task is run and the build timestamp file is present.'

	raise "TestTrack's build_timestamp is not formatted properly." unless BUILD_TIMESTAMP_REGEX.match?(_build_timestamp)
	raise "./testtrack/build_timestamp is not a valid ISO 8601 timestamp, got '#{_build_timestamp}'" unless BUILD_TIMESTAMP_REGEX.match?(_build_timestamp)

	@build_timestamp.presence \|\| raise('build_timestamp is not defined. Ensure `load_build_timestamp` is run.')
	@build_timestamp.presence \|\| raise('build_timestamp is not defined because `load_build_timestamp` initializer was not run.')

		@@ -0,0 +1 @@
		TestTrack.load_build_timestamp unless ENV['SKIP_TIMESTAMP_INIT'] == '1'

	desc 'Generates build timestamp for consuming applications to build versioned requests to split registry'
	desc 'Generates build timestamp for fetching point-in-time split registries'

Add support for timestamped split registry endpoint #97

Add support for timestamped split registry endpoint #97

Conversation

reggieag commented Jan 31, 2020

nanda-prbot commented Jan 31, 2020

aburgel commented Feb 1, 2020

nanda-prbot commented Feb 1, 2020

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

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

aburgel commented Feb 1, 2020

nanda-prbot commented Feb 1, 2020

phantomphildius commented Feb 14, 2020

nanda-prbot commented Feb 14, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aburgel commented Feb 19, 2020

nanda-prbot commented Feb 19, 2020

phantomphildius commented Feb 21, 2020

nanda-prbot commented Feb 21, 2020

Choose a reason for hiding this comment

smudge Feb 22, 2020 • edited

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

smudge commented Feb 22, 2020

nanda-prbot commented Feb 22, 2020

Choose a reason for hiding this comment

phantomphildius commented Apr 11, 2020

nanda-prbot commented Apr 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smudge Feb 22, 2020 •

edited