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

Pass endpoint_id and path_parts to transport #2457

Merged
merged 2 commits into from
Mar 7, 2024

Conversation

pquentin
Copy link
Member

@pquentin pquentin commented Feb 22, 2024

As part of #2435, the transport needs to know the endpoint id and path parts, and can only get them from the client. This pull request makes sure to pass that info to the client.

There are two commits here:

  • 5ff3169 (#2457) which accepts endpoint_id/path_parts in base clients, updates tests and depends on the main branch of elastic-transport
  • 2ce3c32 (#2457) that runs code generation to define endpoint_id/path_parts in all APIs

This PR depends on elastic/elastic-transport-python#151.

I'm wondering about the type of those new parameters. The natural way in elastic-transport was str | DefaultType while here it's str | None. DEFAULT is a way to make None meaningful, but that's not needed here.

Copy link

A documentation preview will be available soon.

Request a new doc build by commenting
  • Rebuild this PR: run docs-build
  • Rebuild this PR and all Elastic docs: run docs-build rebuild

run docs-build is much faster than run docs-build rebuild. A rebuild should only be needed in rare situations.

If your PR continues to fail for an unknown reason, the doc build pipeline may be broken. Elastic employees can check the pipeline status here.

@pquentin pquentin changed the title Pass endpoint_id and path_part to transport Pass endpoint_id and path_parts to transport Feb 22, 2024
@pquentin
Copy link
Member Author

pquentin commented Feb 23, 2024

The previous approach was incorrect for a few reasons:

  • the typing of endpoint_id and path_parts has to be fixed to match the definition in elastic-transport-python. It's not Optional, but Union[DefaultType, ...]. Maybe Optional was fine, we use both styles in the code, not sure why.
  • the values should be quoted (so that eg. ["a", "b"] becomes "a,b")
  • we should only pass values that are set, indeed many APIs have multiple forms, like _cat/recovery and _cat/recovery/{index}

The last two can be solved by defining __path_parts alongside __path, like this:

if index not in SKIP_IN_PATH:
    __path_parts = {"index": _quote(index)}
    __path = f"/_cat/recovery/{__path_parts['index']}"
else:
    __path_parts = {}
    __path = "/_cat/recovery"

The above has been implemented now.

@pquentin pquentin force-pushed the opentelemetry-params branch from b93fbe0 to 2ce3c32 Compare March 6, 2024 08:00
@@ -1,4 +1,5 @@
elastic-transport>=8.0.0b1, <9
# TODO switch back to elastic-transport>=8,<9 between elastic-transport release and elasticsearch-py release
elastic-transport @ git+https://github.com/elastic/elastic-transport-python
Copy link
Member

Choose a reason for hiding this comment

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

I vaguely remember some pain in specifying git urls without an hash, like when trying to upgrade a package checked out from git.
BTW you didn't release a elastic-transport-python because you want to check that all layers are working right?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right. I don't intend to release anything with git dependencies. Maybe I should use a pre-release instead though!

@pquentin pquentin merged commit 45518b0 into elastic:main Mar 7, 2024
11 checks passed
@pquentin pquentin deleted the opentelemetry-params branch March 7, 2024 10:30
Copy link

The backport to 8.13 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-8.13 8.13
# Navigate to the new working tree
cd .worktrees/backport-8.13
# Create a new branch
git switch --create backport-2457-to-8.13
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 45518b0d1201862981201131f76c4b12246ec293
# Push it to GitHub
git push --set-upstream origin backport-2457-to-8.13
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-8.13

Then, create a pull request where the base branch is 8.13 and the compare/head branch is backport-2457-to-8.13.

pquentin added a commit that referenced this pull request Mar 11, 2024
* Accept endpoint_id/path_parts in base clients

* Run code generation

(cherry picked from commit 45518b0)
Copy link

The backport to 8.12 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-8.12 8.12
# Navigate to the new working tree
cd .worktrees/backport-8.12
# Create a new branch
git switch --create backport-2457-to-8.12
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 45518b0d1201862981201131f76c4b12246ec293
# Push it to GitHub
git push --set-upstream origin backport-2457-to-8.12
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-8.12

Then, create a pull request where the base branch is 8.12 and the compare/head branch is backport-2457-to-8.12.

pquentin added a commit that referenced this pull request Mar 11, 2024
* Accept endpoint_id/path_parts in base clients

* Run code generation

(cherry picked from commit 45518b0)
pquentin added a commit that referenced this pull request Mar 11, 2024
* Accept endpoint_id/path_parts in base clients

* Run code generation

(cherry picked from commit 45518b0)
@pquentin
Copy link
Member Author

8.13 backport is #2468. This should not be backported to 8.12, I nearly forgot.

pquentin added a commit that referenced this pull request Mar 11, 2024
…2468)

* Pass endpoint_id and path_parts to transport (#2457)

* Accept endpoint_id/path_parts in base clients

* Run code generation

(cherry picked from commit 45518b0)

* Run code generation
iuliaferoli pushed a commit to iuliaferoli/elasticsearch-py that referenced this pull request Mar 24, 2024
* Accept endpoint_id/path_parts in base clients

* Run code generation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants