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

Add 'experimental/python' support #2052

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

Conversation

kanterov
Copy link
Contributor

@kanterov kanterov commented Dec 30, 2024

Changes

Add experimental/python section replacing experimental/pydabs.

Add 2 new mutators into existing pipeline:

  • ApplyPythonMutator(load_resources) - loads resources from Python code
  • ApplyPythonMutator(apply_mutators) - transforms existing resources defined in Python/YAML

Example:

experimental:
  python:
    resources:
    - "resources:load_resources"
    mutators:
    - "mutators:add_email_notifications"

Tests

Unit tests and manually

@@ -441,9 +547,9 @@ func createInitOverrideVisitor(ctx context.Context) merge.OverrideVisitor {
}

func isOmitemptyDelete(left dyn.Value) bool {
// PyDABs can omit empty sequences/mappings in output, because we don't track them as optional,
// Python can omit empty sequences/mappings in output, because we don't track them as optional,
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the original sentence more clear, "Python can omit empty sequences/mappings in output" does not make sense, as I assume Python is CPython binary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 We are getting rid of "PyDABs" name, so we can't use it anymore. I've rephrased it the same way as it was in the unit test.

@kanterov
Copy link
Contributor Author

@pietern @denik please review, for some reason I can't assign reviewer through UI

@andrewnester andrewnester requested a review from pietern December 30, 2024 15:19
"pattern": "\\$\\{(var(\\.[a-zA-Z]+([-_]?[a-zA-Z0-9]+)*(\\[[0-9]+\\])*)+)\\}"
}
]
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Please execute "make schema" to update this.

Copy link
Contributor

@lennartkats-db lennartkats-db left a comment

Choose a reason for hiding this comment

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

Minor comments, otherwise LGTM

bundle/config/experimental.go Show resolved Hide resolved
bundle/config/mutator/python/python_mutator.go Outdated Show resolved Hide resolved
pietern added a commit that referenced this pull request Jan 2, 2025
## Changes

I noticed a diff in the schema in #2052.

This check should be performed automatically.

## Tests

This PR includes a commit that changes the schema to check that the
workflow actually fails.
@kanterov kanterov force-pushed the kanterov/new-python-support branch from 673b7fd to 29d5da2 Compare January 7, 2025 08:47
@@ -137,7 +225,7 @@ func createCacheDir(ctx context.Context) (string, error) {
// support the same env variable as in b.CacheDir
if tempDir, exists := env.TempDir(ctx); exists {
// use 'default' as target name
cacheDir := filepath.Join(tempDir, "default", "pydabs")
cacheDir := filepath.Join(tempDir, "default", "python")
Copy link
Contributor

Choose a reason for hiding this comment

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

For later:

// b.CacheDir doesn't work because target isn't yet selected

This no longer holds in the new world and can be cleaned up.

case PythonMutatorPhaseLoadResources:
return createLoadResourcesOverrideVisitor(ctx), nil
case PythonMutatorPhaseApplyMutators:
return createInitOverrideVisitor(ctx, insertResourceModeDisallow), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

For later: include comments how the first 2 apply to the deprecated mode and the last 2 to the new mode.

@pietern pietern enabled auto-merge January 7, 2025 14:09
@pietern pietern added this pull request to the merge queue Jan 7, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 7, 2025
@pietern
Copy link
Contributor

pietern commented Jan 7, 2025

An issue with the linter prevents the merge.

@kanterov I'm updating the PR to let it merge.

Copy link

github-actions bot commented Jan 7, 2025

An authorized user can trigger integration tests manually by following the instructions below:

Trigger:
go/deco-tests-run/cli

Inputs:

  • PR number: 2052
  • Commit SHA: db0ca6d7c373f3e25b5eec024a85e87bc6e9cde0

Checks will be approved automatically on success.

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.

4 participants