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

Fix upgrade if porter install has failed and add tests to resource processor #4338

Merged
merged 10 commits into from
Feb 10, 2025

Conversation

marrobi
Copy link
Member

@marrobi marrobi commented Feb 7, 2025

Fixes #4281, #4291

  • Handle upgrade if porter installation not found
  • Add tests to runner and command creation (including move files into helpers directory as resources not liked by pytest).
  • Ensure airlock tests are discovered.

Copy link

github-actions bot commented Feb 7, 2025

Unit Test Results

21 tests   21 ✅  0s ⏱️
 1 suites   0 💤
 1 files     0 ❌

Results for commit e686b4b.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 9 out of 12 changed files in this pull request and generated 1 comment.

Files not reviewed (3)
  • .devcontainer/devcontainer.json: Language not supported
  • resource_processor/resources/helpers.py: Evaluated as low risk
  • resource_processor/vmss_porter/runner.py: Evaluated as low risk
Comments suppressed due to low confidence (2)

resource_processor/helpers/commands.py:82

  • Concatenation of porter_parameters should use += instead of reassignment for better performance and readability.
porter_parameters = f"{porter_parameters} --param {parameter_name}='{parameter_value}'"

resource_processor/helpers/commands.py:98

  • Unnecessary f-string in command_line[0] assignment. It should be command_line[0] += '--force-upgrade '.
command_line[0] = command_line[0] + f"{'--force-upgrade '}"

@marrobi marrobi requested review from Copilot and LizaShak February 10, 2025 13:44
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 9 out of 13 changed files in this pull request and generated 1 comment.

Files not reviewed (4)
  • .devcontainer/devcontainer.json: Language not supported
  • resource_processor/resources/helpers.py: Evaluated as low risk
  • resource_processor/helpers/statuses.py: Evaluated as low risk
  • resource_processor/_version.py: Evaluated as low risk
Comments suppressed due to low confidence (1)

resource_processor/helpers/commands.py:97

  • The --force-upgrade flag should be added to the command correctly. Consider appending it to the command_line list.
if msg_body['action'] == 'upgrade':

@marrobi
Copy link
Member Author

marrobi commented Feb 10, 2025

/test-extended

@marrobi marrobi enabled auto-merge (squash) February 10, 2025 14:53
Copy link

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/13243663390 (with refid 49cbdde2)

(in response to this comment from @marrobi)

1 similar comment
Copy link

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/13243663390 (with refid 49cbdde2)

(in response to this comment from @marrobi)

@marrobi
Copy link
Member Author

marrobi commented Feb 10, 2025

Failing on airlock with:

authenticationerrordetail:Signature not valid in the specified time frame: Start [Mon, 10 Feb 2025 22:05:08 GMT] - Expiry [Mon, 10 Feb 2025 23:20:08 GMT] - Current [Mon, 10 Feb 2025 23:20:33 GMT]

Seems like compute clock is an hour wrong or something. Will force approve as has deployed the workspaces etc, and watch for this again.

@marrobi
Copy link
Member Author

marrobi commented Feb 10, 2025

/test-force-approve

Copy link

🤖 pr-bot 🤖

✅ Marking tests as complete (for commit e686b4b)

(in response to this comment from @marrobi)

@marrobi marrobi merged commit a6d85e2 into microsoft:main Feb 10, 2025
12 checks passed
@marrobi marrobi deleted the marrobi/issue4281 branch February 10, 2025 23:51
@marrobi
Copy link
Member Author

marrobi commented Feb 10, 2025

/test-destroy-env

Copy link

Destroying PR test environment (RG: rg-tre49cbdde2)... (run: https://github.com/microsoft/AzureTRE/actions/runs/13252796384)

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.

Porter: "Installation not found" when preceeding pipeline step fails
3 participants