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

Use email api #252

Closed
wants to merge 4 commits into from
Closed

Conversation

gbonnefille
Copy link
Contributor

This pull-request initiate a first usage of new API endpoints.

Using the official API can bring many interesting feature.

First; sing the official API will reduce code in this role. And less code == better code.

Furthermore, using an official API remove the need to deploy the dedicated scripts. As such deploy can be annoying depending on the overall deployment (native, docker, docker-compose, kubernetes), this will offer the ability to reuse easier this role in much more context.

Cf. #86 and #172

Using the official API will reduce code in this role.
Furthermore, using an official API remove the need to deploy
dedicated scripts. And such deploy can be annoying depending
on the overall deployment (native, docker, docker-compose, kubernetes).
So, using official API will offer the ability to reuse easier this role
in much more context.
@zeitounator zeitounator self-requested a review February 25, 2020 17:18
@zeitounator zeitounator self-assigned this Feb 25, 2020
@zeitounator
Copy link
Member

Thanks for this PR and for your interest. I will review shortly.

Using the API wherever possible is on my todo list. Not only to reduce code in the role (which has already drastically decreased in terms of groovy scripts), but also because sonatype change their internal API quite often (at least on the last releases....) which breaks the existing scripts.

An obvious candidate is the setup_ldap script which is blindly using an unpublished internal API...

Meanwhile, there are some place places in which we might want to keep the scripts for performance reasons (e.g. creating repos from a list, where the api will need a call for each repo). Furthermore, it is not always easy to reach idempotency with rest API without being very verbose.

@gbonnefille
Copy link
Contributor Author

An obvious candidate is the setup_ldap script which is blindly using an unpublished internal API...

I already have a working code for that. I can submit an other PR.

Meanwhile, there are some place places in which we might want to keep the scripts for performance reasons (e.g. creating repos from a list, where the api will need a call for each repo). Furthermore, it is not always easy to reach idempotency with rest API without being very verbose.

I'm currently trying to create repos from API...
For the LDAP, I started idempotency. Effectivelly, it is verbose and require many API calls.

Ideally, a plugin able to read a complete description of the expected status would be better for provisioning. I also found nexus-claim an external Go program to configure Nexus. Could be an other solution instead of trying to recreate everything in Ansible. I don't know currently.

Copy link
Member

@zeitounator zeitounator left a comment

Choose a reason for hiding this comment

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

Apart from context comment about the variable.... The idea is definitely going in the correct direction. But if we have to fully walk this path, I'd rather start working on a set of custom modules with a common lib to manage communication with the api and ensure idempotency.

If we go on just calling the module for each possible operation, the result will be worse than the current groovy script implementation and, on the contrary of your initial statement, will augment the amount of code in the role.

I have to think a bit about all that.

@@ -72,6 +72,7 @@ nexus_public_hostname: 'nexus.vm'
nexus_public_scheme: https

# How should the role access the API for provisionning
nexus_rest_api_endpoint_base: "service/rest"
Copy link
Member

Choose a reason for hiding this comment

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

This name is conflicting/confusing. There is a version based detection of the rest endpoint in nexus_install.yml which sets nexus_rest_api_endpoint used in call_script.yml, declare_script_each.ymlandnexus_install.yml`.

Unfortunately, at this point, the var is pointing directly to the script endpoint (which is historically the only one used).

To clean-up the role's code, I'm considering dropping support for nexus version < 3.11 which would remove this detection (along wiht some other path's detection) and would let us refactor the existing files to use a base as above for all api interactions.

@gbonnefille
Copy link
Contributor Author

I pushed an other example of conversion (LDAP) in #254. I hope that help to design the next step for this role.

@zeitounator zeitounator added the enhancement Related to new features, performance.... label Feb 27, 2020
@gbonnefille
Copy link
Contributor Author

Any news on this topic?

@zeitounator zeitounator added the on hold Changes / decision / precisions / time needed label Oct 23, 2022
zeitounator added a commit that referenced this pull request Dec 16, 2023
Use official API for setting email instead of groovy script

Fixes #252
Fixes #282
---------

Co-authored-by: Guilhem Bonnefille <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Related to new features, performance.... on hold Changes / decision / precisions / time needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants