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

Adds existing fixtures to context and change usages accordingly #3437

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Adds existing fixtures to context and change usages accordingly #3437

wants to merge 14 commits into from
+73 −33

Conversation

pritishpai
Copy link
Contributor

@pritishpai pritishpai commented Dec 11, 2024 •
edited
Loading

Changes

Add fixtures to context and change usages accordingly

Linked issues

Resolves #3428 and #3429

Functionality

  • refactored tests

Tests

  • use existing integration tests

@pritishpai pritishpai requested a review from a team as a code owner December 11, 2024 18:27
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 11, 2024 •
edited
Loading

✅ 65/65 passed, 1 flaky, 3 skipped, 1h9m30s total

Flaky tests:

  • 🤪 test_running_real_migrate_groups_job (3m54.594s)

Running from acceptance #7820

@pritishpai
Copy link
Contributor Author

The pylint disable is unavoidable here as we are adding more fixtures to the context.

JCZuurmond
JCZuurmond requested changes Dec 12, 2024
View reviewed changes
Copy link
Member

@JCZuurmond JCZuurmond left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes LGTM; have another request. See #3429 (not much context there though)

tests/integration/workspace_access/test_workflows.py Outdated Show resolved Hide resolved
@JCZuurmond
Copy link
Member

The pylint disable is unavoidable here as we are adding more fixtures to the context.

@gueniai can bypass the CI to circumvent the "no-cheat" one. (After we reviewed and approved the code.)

@JCZuurmond JCZuurmond added the internal this pull request won't appear in release notes label Dec 12, 2024
@JCZuurmond JCZuurmond mentioned this pull request Dec 13, 2024
Merged
1 task
github-merge-queue bot pushed a commit that referenced this pull request Dec 13, 2024
@JCZuurmond
Exclude TACL migration in table migration integration tests (#3446)
ba5872f 
## Changes
Exclude TACL migration in table migration integration tests because
these were not asserted, and to speed up the tests and reduce flakiness

### Linked issues

Attempt to reduce flakiness blocking CI in #3239
Similar to #3437 in the sense that both PR scope integration tests to a
smaller set of resources

### Tests

- [x] modified integration tests
JCZuurmond
JCZuurmond reviewed Dec 16, 2024
View reviewed changes
Copy link
Member

@JCZuurmond JCZuurmond left a comment

Choose a reason for hiding this comment

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

see comments

tests/integration/conftest.py Outdated Show resolved Hide resolved
tests/integration/conftest.py Outdated Show resolved Hide resolved
JCZuurmond
JCZuurmond approved these changes Dec 20, 2024
View reviewed changes
Copy link
Member

@JCZuurmond JCZuurmond left a comment

Choose a reason for hiding this comment

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

LGTM, before merging make sure to run the full integration test suite for your branch as these changes effect many tests:

  1. Go to https://github.com/databrickslabs/ucx/actions/workflows/nightly.yml
  2. Use the "Run workflow" drop down
  3. Select your branch
  4. Run the CI

@pritishpai
Use make_cluster_policy and make_cluster_policy_permission already ex…
4076174 
…isting in installation context
@pritishpai
Add make_secret_scope and make_secret_scope_acl to installation conte…
7175017 
…xt and refactor usage in tests
@pritishpai
fmt changes
5ff976b
@pritishpai
remove TODO
3d7db0c
@pritishpai
Move include_object_permissions to context instead of the hacky __d…
046e4e4 
…ict__ approach.
@pritishpai
fmt
4762a57
@pritishpai
add object permissions for queries
e5e5d71
@pritishpai
introduce method to set object permissions
60aed71
@pritishpai
fmt
270889c
@pritishpai
add support for remaining objects
9b6430c
@pritishpai
add new attribute to be added ot the config for object permissions
20d9dca
@pritishpai
assign the new property to include_object_permissions
3615a0c
@pritishpai
remove older include_object_permissions
b40e293
@pritishpai
remove dict hack for include_group_names as created_groups property a…
623205e 
…lready exists
@pritishpai
Copy link
Contributor Author

Nightly passed on 2nd attempt https://github.com/databrickslabs/ucx/actions/runs/12435607581

The pylint disable is unavoidable here as we are adding more fixtures to the context.

@gueniai can bypass the CI to circumvent the "no-cheat" one. (After we reviewed and approved the code.)

@gueniai need your approval to bypass the "no-check" check

@pritishpai pritishpai enabled auto-merge December 20, 2024 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JCZuurmond JCZuurmond Awaiting requested review from JCZuurmond

Assignees

@pritishpai pritishpai

Labels
internal this pull request won't appear in release notes
Projects
UCX
Status: Ready for Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[TODO] Move make_secret_scope and make_secret_scope_acl to context like other make_ methods
2 participants
@pritishpai @JCZuurmond