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 artifactory_catalog_connector #99

Merged
merged 10 commits into from
Jul 20, 2022

Conversation

thesuperzapper
Copy link
Member

What changes were proposed in this pull request?

  • Adds a catalog connector for Artifactory
  • See component-catalog-connectors/artifactory-connector/README.md for information about usage
  • Screenshot:
    Screen Shot 2022-03-02 at 11 45 04

How was this pull request tested?

  • The connector has been tested using a real-world Artifactory repo

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

@thesuperzapper thesuperzapper requested a review from ptitzler March 2, 2022 00:53
@thesuperzapper
Copy link
Member Author

thesuperzapper commented Mar 2, 2022

@akchinSTC or @ptitzler, this PR adds the ability to store components in Artifactory repos.

I am not sure if its best suited as an example (in this repo), or as a part of the core Elyra.

@thesuperzapper thesuperzapper force-pushed the artifactory-connector branch from f07e010 to 84b8462 Compare March 2, 2022 01:18
@ptitzler
Copy link
Member

ptitzler commented Mar 2, 2022

I haven't had a chance to review the implementation in detail, but glancing over the files this looks good!

Timing is a bit unfortunate though, because we've just changed the connector API. Those changes are minor but required, since the old API is marked as deprecated in the upcoming release and support will be removed in the 4.0 release. I'll add comments to indicate what those changes are. (We went through this with the core connectors, e.g. elyra-ai/elyra#2520 and elyra-ai/elyra#2518)

I am not sure if its best suited as an example (in this repo), or as a part of the core Elyra.

We've discussed this in our daily dev meeting this morning. You have two options:

Contribute the connector to https://github.com/elyra-ai/elyra

If you agree to maintain the connector going forward, this would be our preferred option. This would result in a couple additional TODO items:

If you would like to take this path let me know and I'll provide additional details.

Publish the connector in any third-party github organization

Since non-core connectors are provided as-is, no additional work (aside from accommodating the API changes and moving this to another repository) would be required.

@thesuperzapper
Copy link
Member Author

thesuperzapper commented Mar 3, 2022

@ptitzler I think Elyra should be careful about including packages that connect to external systems in the core libraries, as this means updates/fixes are then only pushed in new Elyra versions. (Airflow recently went the opposite direction, and has split all the old operators into "providers" which are versioned separately)

But in this specific case (given how prolific Artifactory is) it probably makes sense to include the connector upstream, but I will leave this decision up to you. I am happy to help maintain it, but I somewhat doubt there will be significant changes, as this connector is using Artifactory APIs that haven't changed in over 12 years!

What should I do to get this merged upstream (feel free to just steal the code if you can make the PR faster).


I expect that if we include Artifactory and GitHub connectors upstream, they will make up > 90% of Elyra component catalogs. (NOTE: A GitHub connector would explore a repo for component YAML, and return them using the raw.githubusercontent.com/xxxx URL)

EDIT: I just realized, another important catalog connector will be S3 buckets

@ptitzler
Copy link
Member

ptitzler commented Mar 4, 2022

Just for reference: built-in connectors are currently located several locations:

Even though the proposed connector implementation is KFP specific, it should be treated as a generic connector because the underlying catalog is not runtime specific. If we were to continue using the current file system layout, the source code would need to be moved to a subdirectory in https://github.com/elyra-ai/elyra/tree/master/elyra/pipeline.

@ptitzler
Copy link
Member

ptitzler commented Mar 9, 2022

These should be a complete list of 'migration' steps. The Airflow links are included for illustrative purposes and reflect what was done for https://github.com/elyra-ai/elyra/tree/master/elyra/pipeline/airflow/provider_package_catalog_connector when that connector was migrated:

@thesuperzapper thesuperzapper force-pushed the artifactory-connector branch from 84b8462 to e0b074c Compare June 1, 2022 05:16
@thesuperzapper
Copy link
Member Author

@ptitzler sorry about the long delay, but this is ready to merge now.

I have migrated to the new Elyra 3.7 API for component catalogs, and added LOTS of unit tests!

PS: this is probably stable enough to go into upstream Elyra, but it is fairly specific to Artifactory so may not be widely used. Either way, to get this out there, let's release it under this repo initially.

PSS: we can probably use the same approach as this connector for a possible GitHub connector (see elyra-ai/elyra#2139)

@thesuperzapper thesuperzapper force-pushed the artifactory-connector branch from 0bd04dc to a88cfe9 Compare June 3, 2022 06:01
@thesuperzapper thesuperzapper requested a review from ptitzler June 3, 2022 06:08
@ptitzler
Copy link
Member

ptitzler commented Jun 8, 2022

Overall the functionality, documentation, and tests look good!

@shalberd
Copy link
Contributor

shalberd commented Jun 27, 2022

Does this support https calls with enterprise-internal public key infrastructure with non-publicly trusted CAs for SSL? I am asking in regard to elyra-ai/elyra#2797 Conceptually, https://medium.com/ibm-data-ai/getting-started-with-apache-airflow-operators-in-elyra-aae882f80c4a . In general, how can non-publicly-trusted CAs be brought into the equation here best?

@thesuperzapper thesuperzapper requested a review from ptitzler July 18, 2022 05:14
@thesuperzapper
Copy link
Member Author

@ptitzler I have addressed the outstanding issues you raised, can you take a quick look and see if we can finally get this merged!

Copy link
Member

@ptitzler ptitzler left a comment

Choose a reason for hiding this comment

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

LGTM!

@ptitzler ptitzler merged commit 64994ae into elyra-ai:main Jul 20, 2022
@ptitzler
Copy link
Member

I'll publish the package on PyPI later today.

@ptitzler
Copy link
Member

It's now live at https://pypi.org/project/elyra-artifactory-catalog-connector/0.1.0/. If you provide me with your PyPI user ID I'll add you as a maintainer.

@thesuperzapper
Copy link
Member Author

@ptitzler its the same as my github thesuperzapper

@ptitzler
Copy link
Member

Thanks! An invitation from PyPI should arrive shortly.

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.

3 participants