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

Use SQS_PREFIX not ES_PREFIX for queues #1205

Merged
merged 3 commits into from
Jul 2, 2024
Merged

Conversation

richardhallett
Copy link
Contributor

Purpose

Now that I want to remove the use of ES_PREFIX as we have a specific new ES server domain.
Then some detection needs to change, namely as ES_PREFIX will now be empty we need a way to choose queues.

Approach

The way im working this is to add a new SQS_QUEUE which I will then set configured in terraform.
You will note that I'm still use a weird detection for some salesforce message part, this is because this entire code is very strange and I'm not sure I want to break it further, so for now I just use SQS_QUEUE as the way to identify here.

I do note that while Rails has technically two separate environments setup for stage and test, we're not actually using this properly, I'm afraid of making that change as it seems more involved, but is probably ideally the better solution for identifying the environment.

Open Questions and Pre-Merge TODOs

N/A

Learning

N/A

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

  • New feature (non-breaking change which adds functionality)

  • Breaking change (fix or feature that would cause existing functionality to change)

Reviewer, please remember our guidelines:

  • Be humble in the language and feedback you give, ask don't tell.
  • Consider using positive language as opposed to neutral when offering feedback. This is to avoid the negative bias that can occur with neutral language appearing negative.
  • Offer suggestions on how to improve code e.g. simplification or expanding clarity.
  • Ensure you give reasons for the changes you are proposing.

@richardhallett richardhallett requested a review from a team July 2, 2024 16:00
@richardhallett richardhallett self-assigned this Jul 2, 2024
@@ -32,11 +32,11 @@ module Indexable
elsif instance_of?(Event)
OtherDoiJob.perform_later(dois_to_import)
# ignore if record was created via Salesforce API
elsif instance_of?(Provider) && !from_salesforce && (Rails.env.production? || ENV["ES_PREFIX"] == "stage")
elsif instance_of?(Provider) && !from_salesforce && (Rails.env.production? || ENV["SQS_PREFIX"] == "stage")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the PR Description, but yes this isn't much better, but arguably not worse

Copy link
Member

@digitaldogsbody digitaldogsbody left a comment

Choose a reason for hiding this comment

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

One question: is the rails env definitely called test (as opposed to testing)? I don't remember, hence the question, but if it is testing then I think some of this will fall over (such as the SQS queue names).

Other than that, happy for merge :)

@richardhallett
Copy link
Contributor Author

It's test but it's not set to that, the test environments are set to stage which is not helpful. Hence why there was some code in there that was doing t hings like if Rails.env.stage? but then also asking ES_PREFIX because it can't differentiate between staging and test.

So for the container config for stage SQS_PREFIX will be "stage"
And for test SQS_PREFIX will be "test"
And for production it will just fall back to environment and be "production"

I did think maybe I could just not fall back to the Rails.env entirely and just be explicit about it, but I sort of went for a half and half compromise for how it was already working.

@richardhallett richardhallett merged commit a1c2aa9 into master Jul 2, 2024
13 checks passed
@richardhallett richardhallett deleted the fix_stage_detection branch July 2, 2024 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants