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.

Sign up for GitHub

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

Jump to bottom

acc: replace LocalOnly option with Local & Cloud #2387

Merged
merged 10 commits into from
Feb 26, 2025
Merged

acc: replace LocalOnly option with Local & Cloud #2387

merged 10 commits into from
Feb 26, 2025
+29 −25

Conversation

denik
Copy link
Contributor

@denik denik commented Feb 26, 2025 •
edited
Loading

Changes

Instead of LocalOnly with non-composable semantics there are two composable options:

  • Local - enable test locally
  • Cloud - enable test on the cloud

By default Cloud is switched off except in bundle (but not in bundle/variables and bundle/help).

Tests

Using this in #2383 to have test that runs on cloud but not locally.

@denik denik temporarily deployed to test-trigger-is February 26, 2025 14:41 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is February 26, 2025 14:43 — with GitHub Actions Inactive
andrewnester
andrewnester reviewed Feb 26, 2025
View reviewed changes
Copy link
Contributor

@andrewnester andrewnester left a comment

Choose a reason for hiding this comment

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

By default Cloud is switched off except in bundle

What the reason for this?

@denik
Copy link
Contributor Author

denik commented Feb 26, 2025

By default Cloud is switched off except in bundle
What the reason for this?

That just reflects current situation and the kind of tests we have there.

@denik denik temporarily deployed to test-trigger-is February 26, 2025 15:09 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is February 26, 2025 15:09 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is February 26, 2025 15:11 — with GitHub Actions Inactive
@denik denik force-pushed the denik/acc-rm-localonly branch from b1ae401 to e5eea9b Compare February 26, 2025 15:13
@denik denik temporarily deployed to test-trigger-is February 26, 2025 15:13 — with GitHub Actions Inactive
shreyas-goenka
shreyas-goenka approved these changes Feb 26, 2025
View reviewed changes
Copy link
Contributor

@shreyas-goenka shreyas-goenka left a comment

Choose a reason for hiding this comment

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

Thanks! This is nice.

acceptance/test.toml
@@ -0,0 +1,2 @@
Local = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment here that this is the global configuration for all tests and that tests are only run locally by default and not as integration tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a comment

acceptance/bundle/trampoline/warning_message_with_new_spark/test.toml
@@ -1,6 +1,6 @@
# Since we use existing cluster id value which is not available in cloud envs, we need to stub the request
# and run this test only locally
LocalOnly = true
Cloud = false
Copy link
Contributor

Choose a reason for hiding this comment

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

All these layers of override can be difficult to track. Could you add some functionality to see the resolved configuration for a test case or directory as a follow-up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there is an easy way, so you'll have to track those manually. For a given boolean option you only need to know the closest setting though, not the whole chain.

@denik denik temporarily deployed to test-trigger-is February 26, 2025 15:15 — with GitHub Actions Inactive
@denik denik force-pushed the denik/acc-rm-localonly branch from d968bf9 to 8c51b89 Compare February 26, 2025 15:18
@denik denik temporarily deployed to test-trigger-is February 26, 2025 15:18 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is February 26, 2025 15:19 — with GitHub Actions Inactive
@denik denik enabled auto-merge February 26, 2025 15:19
@denik denik temporarily deployed to test-trigger-is February 26, 2025 15:39 — with GitHub Actions Inactive
@denik denik added this pull request to the merge queue Feb 26, 2025
Merged via the queue into main with commit 81606cf Feb 26, 2025
9 checks passed
@denik denik deleted the denik/acc-rm-localonly branch February 26, 2025 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shreyas-goenka shreyas-goenka shreyas-goenka approved these changes

@andrewnester andrewnester andrewnester approved these changes

@pietern pietern Awaiting requested review from pietern pietern is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@denik @andrewnester @shreyas-goenka