HOTT-4931 Drop CDS Env Var #1716

HWallenberg · 2024-02-08T17:38:02Z

Jira link

https://transformuk.atlassian.net/browse/HOTT-4931

What?

I have removed:

CDS Env Var from Taric Importer and Rollback, Apply, Download Workers as they will use uk? var instead
CDS Env Var from Terraform and other config files as it is now redundant

I have updated:

taric_importer as it wasn't recognising filenames in the correct format (YYYY-MM-DD) and file checks were just defaulting due to the erroneous env var use_cds. It will create_update_entry even if there isn't a filename in the correct format using today's date. This is up for discussion as the logic is different for CDS_Importer.
cds_synchronizer>Rollback as removal of use_cds env var meant that rollback tests failed when running as a uk worker.
Specifically test 'it performs a rollback' in spec/controllers/api/admin/rollbacks_controller_uk_spec.rb. It looks like there was never any proper unit tests for this controller as a uk worker. It failed because the number of measures stayed at one instead of zero. So cds_Synchronizer will rollback based on date rather than updates relating to a filename similar to the Taric_synchronizer rollback. Again this is up for discussion and was altered due to uk worker tests failing.
the specs connected with this process as tests were failing (see below for detail)

Why?

I am doing this because:

The use_cds variable is redundant. It was used when uk and xi both used the Taric download service. Following a transition uk now uses the CDS service and xi uses Taric. So logic changed to use if the worker is uk?CDS Synchronizer runs so and won't run as a xi worker. A xi worker will use the Taric Synchronizer
Following removal of the use_cds var some tests did not work as expected when running as either a uk/xi worker. Some tests became either CDS or Taric specific (by default) depending on the test and the redundant use_cds env var.
Originally there was one spec called spec/controllers/api/admin/rollbacks_controller_spec.rb which was not CDS(uk)/Taric(xi) specific so it is now split into two specs.
I have locally tested Rollback, Apply and rake:download_apply_and_reindex for a uk worker and xi worker and both worked as expected locally.

Deployment risks

Includes actions which may effect critical data in the daily Taric/CDS sync jobs.

…ork as expected

… incorrect worker service

… code and test

…for cds rollback

…h rollback tests

…perform a rollback as this has been refactored to unit tests in cds_synchronizer and taric_synchronizer.

HWallenberg · 2024-03-05T15:04:56Z

app/lib/taric_importer.rb

@@ -26,6 +26,7 @@ def import
    return unless proceed_with_import?(filename)


These changes may not be required as Taric Updates and importing is processed in app/lib/tariff_synchronizer/taric_update_downloader.rb. It could be that this code is actually redundant now...

…nd a file must be set to NIL for base_update_importer_spec.rb to raise exceptions

jebw

Looks great - only query is that create_update_entry which used to be dead code and I'm pretty sure should continue to be dead code

jebw · 2024-02-09T10:39:30Z

spec/unit/tariff_synchronizer/base_update_importer_spec.rb

@@ -4,6 +4,11 @@
  let(:base_update_importer) { described_class.new(taric_update) }

  describe '#apply', truncation: true do
+    before do
+      allow(TradeTariffBackend).to receive(:uk?).and_return(false)


Should this set the service to xi? as it stands both uk? and xi? will return false i think?

allow(TradeTariffBackend).to receive(:service).and_return('xi')

jebw · 2024-02-09T10:45:26Z

spec/workers/rollback_worker_spec.rb


+      it 'invokes rollback' do
+        described_class.new.perform(date)


This line is the same for every test so could get moved to the before block?

spec/controllers/api/admin/rollbacks_controller_uk_spec.rb

spec/controllers/api/admin/rollbacks_controller_xi_spec.rb

jebw · 2024-03-08T11:19:27Z

app/lib/taric_synchronizer.rb

@@ -122,7 +126,7 @@ def rollback(rollback_date, keep: false)
          end
        end

-        Rails.logger.info "Rolled back to #{date}. Forced keeping records: #{!!keep}"
+        Rails.logger.info "Rolled back to #{date}. Forced keeping records: #{!keep.nil?}"


This looks odd? means that keep == false will print 'Force keeping records: true' , as will keep == true

jebw · 2024-03-08T11:23:40Z

spec/unit/tariff_synchronizer/base_update_importer_spec.rb

@@ -6,6 +6,9 @@
  describe '#apply', truncation: true do
    before do
      allow(TradeTariffBackend).to receive(:service).and_return('xi')
+      # Setting :FIND to NIL as Taric_Importer.rb checks a file doesn't exist to Proceed_With_Import?
+      # SEE SPECS ON LINES 39 & 58.
+      allow(TariffSynchronizer::TaricUpdate).to receive(:find).and_return(nil) # Assuming no existing record


We need to discuss this, I'm assuming from what you're saying that there are records in the tariff_updates table in which case I think those should be getting removed instead (or never created) but I may be missing something

jebw · 2024-03-08T11:24:08Z

spec/unit/taric_synchronizer_spec.rb

@@ -148,6 +149,7 @@
      before do
        failed_update
        allow(TradeTariffBackend).to receive(:service).and_return('xi')
+        allow(TradeTariffBackend).to receive(:service).and_return('xi')


Duplicate line

jebw · 2024-03-08T11:24:18Z

spec/unit/taric_synchronizer_spec.rb

@@ -101,6 +101,7 @@
        allow_any_instance_of(TaricImporter).to receive(:import)
        allow(TariffSynchronizer::TariffLogger).to receive(:failed_update)
        allow(TradeTariffBackend).to receive(:service).and_return('xi')
+        allow(TradeTariffBackend).to receive(:service).and_return('xi')


Duplicate line

jebw · 2024-03-08T11:27:43Z

spec/integration/taric_synchronizer_spec.rb

@@ -15,6 +15,7 @@

      context 'when everything is fine' do
        it 'applies missing updates' do
+          # Calls taric_importer.rb and needs Create_Update_Entry!


Self referential description loop 😉

Class says to see here for explanation, here effectively says to look at the class

Update gets marked as applied here

https://github.com/trade-tariff/trade-tariff-backend/blob/main/app/lib/tariff_synchronizer/taric_update.rb#L75

jebw · 2024-03-08T11:58:39Z

app/lib/taric_importer.rb

+    #This is required see spec/integration/taric_synchronizer_spec.rb:17
+    create_update_entry(file_path:, filename:)


When I checkout this branch I can comment out this line without impact on specs - not done a sync test though

When I'd look through the codes history, it looks like this was added in the early days of the CDS sync plans - looks like initial they tried to add CDS support to the Taric importer before splitting it into a separate CDS importer.

Presumably it has been dead code since then? create_update_entry, determine_file_size, and determine_filename can all be removed AFAICT

jebw · 2024-03-08T14:56:14Z

app/lib/taric_importer.rb

@@ -58,19 +59,24 @@ def taric_failed_log(exception, hash)
  private

  def proceed_with_import?(filename)


This check always returns true (because use_cds? is false)

So the method can go

This was returning early incorrectly causing spec failures

HWallenberg added 3 commits February 8, 2024 16:38

remove use_cds and update tests and code so CDS Sync and Taric Sync w…

82e8782

…ork as expected

Fix linting errors

968a1fb

Moved DataMigration out of foreach loop

0742c26

HWallenberg requested a review from jebw February 8, 2024 17:53

HWallenberg added 10 commits February 13, 2024 17:18

Update specs to use service and added exception handler for detecting…

497bae1

… incorrect worker service

specs updated for uk worker on a taric process exception handling

76a3715

Fixed rollback tests and linting

9212de5

fixed linting error

ef78254

Removed Taric tests from cds_synchronizer and added wrong environment…

0f21b3f

… code and test

Tidied up wrong environment error specs for Taric

c24b2fb

tidied up taric_synchronizer, added wrong environment code and specs …

dafd070

…for cds rollback

changed CDS_synchronizer back the way it was and updated the spec wit…

380a5b6

…h rollback tests

Renamed uk rollback controller spec back to generic name and removed …

7629b1b

…perform a rollback as this has been refactored to unit tests in cds_synchronizer and taric_synchronizer.

Fixed linting error

97d0c12

HWallenberg commented Mar 5, 2024

View reviewed changes

HWallenberg added 4 commits March 7, 2024 16:05

Merged Main to Feature Branch

54911ac

fix specs following merge conflict resolution

b3c6771

removed CDS from terraform/admin

f58d0f1

Taric Importer uses Create_Update_Entry for Taric_Synchronizer_spec a…

007a649

…nd a file must be set to NIL for base_update_importer_spec.rb to raise exceptions

jebw reviewed Mar 8, 2024

View reviewed changes

jebw added 2 commits March 8, 2024 15:35

HOTT-4931 Removed convoluted dead code

48202cc

This was returning early incorrectly causing spec failures

HOTT-4931 Lint fixes

f34ed5f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HOTT-4931 Drop CDS Env Var #1716

HOTT-4931 Drop CDS Env Var #1716

HWallenberg commented Feb 8, 2024

HWallenberg Mar 5, 2024

jebw left a comment

jebw Feb 9, 2024

jebw Feb 9, 2024

jebw Mar 8, 2024

jebw Mar 8, 2024

jebw Mar 8, 2024

jebw Mar 8, 2024

jebw Mar 8, 2024

jebw Mar 8, 2024

jebw Mar 8, 2024

jebw Mar 8, 2024

		@@ -26,6 +26,7 @@ def import
		return unless proceed_with_import?(filename)

		#This is required see spec/integration/taric_synchronizer_spec.rb:17
		create_update_entry(file_path:, filename:)

		@@ -58,19 +59,24 @@ def taric_failed_log(exception, hash)
		private

		def proceed_with_import?(filename)

HOTT-4931 Drop CDS Env Var #1716

Are you sure you want to change the base?

HOTT-4931 Drop CDS Env Var #1716

Conversation

HWallenberg commented Feb 8, 2024

Jira link

What?

Why?

Deployment risks

Choose a reason for hiding this comment

jebw left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment