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][functions] Fix the download of builtin Functions #17877

Merged
merged 2 commits into from
Sep 30, 2022

Conversation

cbornet
Copy link
Contributor

@cbornet cbornet commented Sep 28, 2022

Motivation

When uploadBuiltinSinksSources is set to false (non-default), the download function is not considering the component type and only looking into the connectors dir.
Also we are scanning the connectors dir instead of using the info from the ConnectorsManager.

Modifications

This change looks at the component type if the download function is called with tenant+ns+name to choose between the ConnectorsManager and the FunctionsManager to get the archive path.

If the download function is called with a direct path, we look first into the ConnectorsManager then into the FunctionsManager.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Run tests in FunctionApiV3ResourceTest

Does this pull request potentially affect one of the following parts:

no

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • Anything that affects deployment

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    fix

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

Matching PR in forked repository

PR in forked repository: cbornet#2

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 28, 2022
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

it would be great to add some tests

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Sep 29, 2022
@cbornet
Copy link
Contributor Author

cbornet commented Sep 29, 2022

it would be great to add some tests

Yes of course. I tested it manually for now. I'll add tests for the download function in a subsequent PR.

The download function was not considering the component type and only looking into the connectors dir. Also we were scanning the connectors dir instead of using the info from the ConnectorsManager. This change looks at the component type if the download function is called with tenant+ns+name to choose between the ConnectorsManager and the FunctionsManager to get the archive path.
If the download function is called with a direct path, we look first into the ConnectorsManager then into the FunctionsManager.
@cbornet cbornet requested review from eolivelli and nicoloboschi and removed request for eolivelli September 29, 2022 21:39
@cbornet
Copy link
Contributor Author

cbornet commented Sep 29, 2022

Added some unit tests. PTAL again.

@nicoloboschi nicoloboschi merged commit 6651bbb into apache:master Sep 30, 2022
@cbornet cbornet deleted the functions-download branch October 14, 2022 09:01
@Technoboy- Technoboy- added this to the 2.12.0 milestone Nov 24, 2022
cbornet added a commit to cbornet/pulsar that referenced this pull request Nov 24, 2022
cbornet added a commit to cbornet/pulsar that referenced this pull request Nov 24, 2022
cbornet added a commit to cbornet/pulsar that referenced this pull request Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants