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

feat: fix task queue trigger for updating reports post release #801

Merged
merged 3 commits into from
Nov 11, 2024
Merged

Conversation

cka-y
Copy link
Contributor

@cka-y cka-y commented Nov 6, 2024

Summary:
Closes #785
This pull request includes a small change to the .github/workflows/validator-update.yml file. The change corrects the queue name format and adds a location variable to the gcloud tasks create-http-task command.

  • Corrected queue name format from update_validation_report_task_queue to update-validation-report-task-queue.
  • Added --location parameter to the gcloud tasks create-http-task command.

Expected behavior:
The task queue is successfully created.
Example for this action run: update-validation-report-task-queue

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Run the unit tests with ./scripts/api-tests.sh to make sure you didn't break anything
  • Add or update any needed documentation to the repo
  • Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
  • Linked all relevant issues
  • Include screenshot(s) showing how this pull request works and fixes the issue(s)

@cka-y cka-y marked this pull request as ready for review November 6, 2024 18:49
@@ -32,7 +32,8 @@ jobs:
- name: Create task to run cloud function
run: |
gcloud tasks create-http-task \
--queue=update_validation_report_task_queue \
--queue=update-validation-report-task-queue \
Copy link
Contributor

@jcpitre jcpitre Nov 8, 2024

Choose a reason for hiding this comment

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

I think having 2 ways to name the queue in main.tf (with underscore and dash) is just confusing. Why can't both be the same?

Or maybe the resource name should be: update_validation_report_task_queue_tf_name or something like this since I think it's only used within terraform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, all our resources are named using underscores, like name_of_resource, whereas individual elements are named with hyphens, such as name-of-element (this naming convention also applies to Python functions). However, when I try to name the queue as name_of_queue, GCloud throws an error.
The gcloud system requires queue IDs to contain only letters, numbers, or hyphens (-), and restricts the use of underscores.

Copy link
Contributor

@jcpitre jcpitre Nov 8, 2024

Choose a reason for hiding this comment

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

FYI there's at least one case where the underscore is used in both  "names" (See there). But it's not a queue.

What about naming them differently? AFAIK the name on the resource line (update_validation_report_task_queue) is used only within terraform. So we could have some kind of suffix.

In any case I guess this might be out of scope for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I guess it would be valuable to go over the naming convention in terraform. Issue created: #802 -- feel free to add some details.

@cka-y cka-y merged commit b680fda into main Nov 11, 2024
4 checks passed
@cka-y cka-y deleted the feat/785 branch November 11, 2024 17:35
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.

Post-release validation report generation failing
2 participants