refactor: implementation for consuming internal or external artifactory #442
Conversation
|
@JoseRamonRodriguez this is PR that can be tested for external use case with saf desktop 1.0.rc3 for now. This will be finally needed to merge with #436 where solution template is being updated by @iazehaf for upgrade to saf-desktop 1.0.0 and other upgrades. |
|
@JoseRamonRodriguez @mayur-lankeshwar |
|
@iazehaf This is a good illustration of the issue we discussed yesterday for the 'source of truth' for the setup-environment.py script. Looking at the changes proposed here, they differ from the latest available in the I also want to highlight that the copy in common-files appears to have a typo introduced by this PR: https://github.com/ansys-internal/solutions-common-files/pull/2 Note the difference between the env var here - @jacobevansgit can you confirm if propagatebot is configured to monitor the setup-environment script in common-files? At quick glance, it looks like yes: https://github.com/ansys-internal/solutions-common-files/blob/d983a4c067c4f280f77d60c6a9fd728e55ea5e46/.github/workflows/propagate_bot.yml#L52-L59 Context: https://github.com/ansys-internal/solution-applications-maintainers/issues/18 |
…suadhyay/func/consume-external-artifactory
…github.com/ansys/ansys-templates into suadhyay/func/consume-external-artifactory
| raise Exception(f"Unknown private source {source['name']} with url {source['url']}.") | ||
| token = os.environ.get("PYANSYS_PYPI_PRIVATE_PAT") | ||
| elif source["url"] == "https://artifactory.ansys.com/artifactory/api/pypi/saf_pypi/simple/": | ||
| token = os.environ.get("ARTIFACTORY_PRIVATE_PYPI_PAT") |
There was a problem hiding this comment.
We should be consistent with the ENV vars - notice the order of the pyansys and solutions vars are PYPI_PRIVATE and the new one is PRIVATE_PYPI
| # Declare credentials for private sources | ||
| for source in private_sources: | ||
| print(f"Declare credentials for {source['name']}") | ||
| USERNAME = "PAT" |
There was a problem hiding this comment.
| USERNAME = "PAT" | |
| username = "PAT" |
| # Declare credentials for private sources | ||
| for source in private_sources: | ||
| print(f"Declare credentials for {source['name']}") | ||
| USERNAME = "PAT" | ||
| token = None | ||
| if source["name"].lower() == "pypi": | ||
| continue | ||
| elif source["url"] == "https://pkgs.dev.azure.com/pyansys/_packaging/pyansys/pypi/simple/": | ||
| token = os.environ["PYANSYS_PYPI_PRIVATE_PAT"] |
There was a problem hiding this comment.
| token = os.environ["PYANSYS_PYPI_PRIVATE_PAT"] | |
| token = os.environ.get("PYANSYS_PYPI_PRIVATE_PAT") |
| # Create Poetry environment variable | ||
| os.environ[f"POETRY_HTTP_BASIC_{source_name}_USERNAME"] = "PAT" | ||
| os.environ[f"POETRY_HTTP_BASIC_{source_name}_PASSWORD"] = token | ||
| if token and USERNAME: |
There was a problem hiding this comment.
This should probably have an else that displays an error
| @@ -932,6 +948,66 @@ def install_dotnet_linux_dependencies(): | |||
| shell=True, | |||
| ) | |||
|
|
|||
| def check_if_user_is_internal_or_external(): | |||
| private_pypi_pats = [ | |||
| os.environ.get("PYANSYS_PRIVATE_PYPI_PAT"), | |||
There was a problem hiding this comment.
These values are duplicated a few times in this file, so we should probably pull them out as variables
| os.environ.get("PYANSYS_PRIVATE_PYPI_PAT"), | ||
| os.environ.get("SOLUTIONS_PRIVATE_PYPI_PAT") | ||
| ] | ||
| is_internal = any(private_pypi_pats) |
There was a problem hiding this comment.
| is_internal = any(private_pypi_pats) | |
| internal_source_configured = any(private_pypi_pats) |
| 'pyansys-private-pypi': {'url': 'https://pkgs.dev.azure.com/pyansys/_packaging/pyansys/pypi/simple/', 'priority': 'supplemental'}, | ||
| 'artifactory-private-pypi': {'url': 'https://artifactory.ansys.com/artifactory/api/pypi/saf_pypi/simple/', 'priority': 'supplemental'} | ||
| } | ||
| private_sources = get_private_sources(DEPENDENCY_MANAGER_PATHS["common"]["configuration_file"]) |
There was a problem hiding this comment.
| private_sources = get_private_sources(DEPENDENCY_MANAGER_PATHS["common"]["configuration_file"]) | |
| toml_private_sources = get_private_sources(DEPENDENCY_MANAGER_PATHS["common"]["configuration_file"]) |
| } | ||
| private_sources = get_private_sources(DEPENDENCY_MANAGER_PATHS["common"]["configuration_file"]) | ||
| private_sources = [source for source in private_sources if source['name'] != 'PyPI'] | ||
| internal_sources = {'solutions-private-pypi', 'pyansys-private-pypi'} |
There was a problem hiding this comment.
The internal vs. private naming is very confusing to me - shouldn't they all be private?
Also, the external part is equally confusing because you still need to be authenticated. For naming, we should either stick to the source (internal/external) or the access (private/public)
| else: | ||
| sources_to_add |= external_sources - all_sources | ||
|
|
||
| commands = [ |
There was a problem hiding this comment.
Personally, I don't like this at all. This means that the TOML file isn't really deterministic - it depends on whatever ENV vars are set when you run the setup script. I think this will result in a lot of unnecessary confusion as developers commit versions of the TOML file switching back and forth between solutions/pyansys vs. artifactory.
I don't know what the requirements are for the artifactory access, so it is difficult for me to propose an alternative solution. I know portal uses 2 TOML files, which I also don't like.
My only other suggestion is to dynamically switch from solutions/pyansys to artifactory during the build/package/release process in CI. However, this would need to be configurable because there may be situations where we want to build a package for internal consumption that depends on prerelease package versions that aren't available in artifactory.
There was a problem hiding this comment.
Thinking out loud - another option is to test how poetry handles private repositories that aren't authenticated. If this is handled gracefully, we could simply add all the potential sources as supplemental sources and let poetry figure out which ones are authenticated properly.
@nick-marnik, correct, however sadly it was not run on the most recent PR change to What your link shows, is the repos that will receive PRs when the bot is run. |
I think we discussed this before - but in situations where we want propagate bot to monitor a given file, can we skip the entire checklist/label/merge part and simply automate propagate bot to run when any PR affecting a given file is merged? |
Description
Internal and External user must be able to consume internal or external artifactory respectively for downloading packages.
Related Tickets
QA Instructions, Screenshots, Recordings
Steps for internal users to consume packages:
Steps for external users to consume packages: