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 namespace parameter in legacy config #241

Merged

Conversation

chouetz
Copy link
Contributor

@chouetz chouetz commented Sep 26, 2023

Hello
I propose to add the namespace in field of the LegacyGalaxyInfo class to solve the following:

I'm trying to validate a legacy role with both galaxy-importer.main and ansible-lint
If I have NO namespace in my meta/main.yaml I have the following error when executing ansible-lint:

ERROR    Computed fully qualified role name of datadog does not follow current galaxy requirements.
Please edit meta/main.yml and assure we can correctly determine full role name:

galaxy_info:
role_name: my_name  # if absent directory name hosting role is used instead
namespace: my_galaxy_namespace  # if absent, author is used instead

Namespace: https://galaxy.ansible.com/docs/contributing/namespaces.html#galaxy-namespace-limitations
Role: https://galaxy.ansible.com/docs/contributing/creating_role.html#role-names

As an alternative, you can add 'role-name' to either skip_list or warn_list.

Computed fully qualified role name of datadog does not follow current galaxy requirements.
Please edit meta/main.yml and assure we can correctly determine full role name:

galaxy_info:
role_name: my_name  # if absent directory name hosting role is used instead
namespace: my_galaxy_namespace  # if absent, author is used instead

Namespace: https://galaxy.ansible.com/docs/contributing/namespaces.html#galaxy-namespace-limitations
Role: https://galaxy.ansible.com/docs/contributing/creating_role.html#role-names

As an alternative, you can add 'role-name' to either skip_list or warn_list.

and when I have a namespace and execute galaxy-importer:

ERROR: The import failed for the following reason: unknown field in galaxy_info

Tests performed with

Importing with galaxy-importer 0.4.13
ansible-lint 6.14.3 using ansible 2.15.4

With this PR I allow having a namespace in the configuration but still require namespace to be provided for galaxy-importer (python -m galaxy_importer.main --legacy-role my-role --namespace my-namespace)

@chouetz chouetz force-pushed the nschweitzer/add_namespace_in_legacy_config branch from 17e8dcc to dd2f1d8 Compare September 26, 2023 09:03
@chouetz chouetz force-pushed the nschweitzer/add_namespace_in_legacy_config branch 2 times, most recently from af2422f to be24382 Compare September 26, 2023 09:16
@chouetz
Copy link
Contributor Author

chouetz commented Sep 27, 2023

@jctanner sorry to bother you again, would this proposal make sense as well?
Many thanks in advance

@jctanner
Copy link
Collaborator

@chouetz legacy namespaces in galaxy_ng aren't actually restricted to the same set of characters as the v3/collection namespaces. We have to allow any valid github username to become a legacy namespace to assert compatibility with all the roles currently on galaxy.ansible.com

@chouetz chouetz force-pushed the nschweitzer/add_namespace_in_legacy_config branch 2 times, most recently from 2d0af6d to c884de4 Compare September 28, 2023 08:17
Add namespace parameter in legacy config
@chouetz chouetz force-pushed the nschweitzer/add_namespace_in_legacy_config branch from c884de4 to 1a8b3d9 Compare September 28, 2023 08:25
@chouetz
Copy link
Contributor Author

chouetz commented Sep 28, 2023

@jctanner I updated the regex. According to what I saw on galaxy.ansible.com we have to enable underscore whereas it's not allowed for github username, I guess.
Many thanks for your feedback

@utoddl
Copy link

utoddl commented Oct 3, 2023

The line above this change, Line 61, has a bad regexp also.

LEGACY_ROLE_NAME_REGEXP = re.compile("^[a-zA-Z0-9-_]{1,55}$")

should be

LEGACY_ROLE_NAME_REGEXP = re.compile("^[a-zA-Z0-9_-]{1,55}$")

As it is, it matches '9' through '_', not underscore or hyphen.

@chouetz chouetz force-pushed the nschweitzer/add_namespace_in_legacy_config branch from 4979904 to a17f996 Compare October 4, 2023 15:40
@chouetz
Copy link
Contributor Author

chouetz commented Oct 4, 2023

The line above this change, Line 61, has a bad regexp also.

LEGACY_ROLE_NAME_REGEXP = re.compile("^[a-zA-Z0-9-_]{1,55}$")

should be

LEGACY_ROLE_NAME_REGEXP = re.compile("^[a-zA-Z0-9_-]{1,55}$")

As it is, it matches '9' through '_', not underscore or hyphen.

Seems it broke one test. I did a modification without checking the content of the eda tarball, I'm not sure this patch is correct. I'll try to deep dive

@chouetz
Copy link
Contributor Author

chouetz commented Oct 10, 2023

@utoddl @jctanner could you please review the PR please?
Many thanks in advance

@utoddl
Copy link

utoddl commented Oct 10, 2023

I approved, @chouetz , but I'm only "grey check" worthy. (I don't have write access.) You'll need an approval from a reviewer with "green check" super powers. :>

@jctanner
Copy link
Collaborator

jctanner commented Oct 10, 2023

I've dumped the role namespaces and names from old-galaxy to check these regexes ...

https://gist.github.com/jctanner/a3573e1583897bd24408c2ac3d115032

Failures: https://gist.githubusercontent.com/jctanner/a3573e1583897bd24408c2ac3d115032/raw/6635bd02e0bc0b71c41326cfa2655c966676f3e9/results.txt

Some are expected failures such as ...

However, I believe we need to allow roles like these ...

  • FAILED - id:3487 namespace:george.shuklin
  • FAILED - id:23881 namespace:pilou-
(venv) [jtanner@p1 galaxy_ng.legacy_role_mapping]$ ansible-galaxy role search -s https://old-galaxy.ansible.com pilou-

Found 1 roles matching your search:

 Name              Description
 ----              -----------
 pilou-.postgresql Install and configure PostgreSQL, dependencies, extensions, databases and users.
(venv) [jtanner@p1 galaxy_ng.legacy_role_mapping]$ ansible-galaxy role search -s https://old-galaxy.ansible.com george.shuklin

Found 4 roles matching your search:

 Name                                      Description
 ----                                      -----------
 george.shuklin.flow-tools                 Install and configure flow-capture from flow-tools package
 george.shuklin.get-external-ip-via-dyndns Set fact about machine - external (white) IPv4 address via opendns server.
 george.shuklin.inventory_to_hostname      Set up hostname according to inventory name
 george.shuklin.ovs-bridges                Create ovs bridges and add interface to it
(venv) [jtanner@p1 galaxy_ng.legacy_role_mapping]$ ansible-galaxy role install -p /tmp/test.roles -s https://old-galaxy.ansible.com george.shuklin.inventory_to_hostname
Starting galaxy role install process
- downloading role 'inventory_to_hostname', owned by george.shuklin
- downloading role from https://github.com/amarao/ansible-inventory_to_hostname/archive/master.tar.gz
- extracting george.shuklin.inventory_to_hostname to /tmp/test.roles/george.shuklin.inventory_to_hostname
- george.shuklin.inventory_to_hostname (master) was installed successfully

galaxy_importer/constants.py Outdated Show resolved Hide resolved
@jctanner jctanner merged commit 98e0931 into ansible:master Oct 11, 2023
5 checks passed
@chouetz chouetz deleted the nschweitzer/add_namespace_in_legacy_config branch October 11, 2023 15:08
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.

3 participants