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 support for helm registry login/logout #601

Closed
wants to merge 8 commits into from

Conversation

abikouo
Copy link
Contributor

@abikouo abikouo commented Mar 28, 2023

SUMMARY

closes #578

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

helm_registry_auth

Copy link

@abikouo abikouo force-pushed the helm_registry_login branch from 210285b to f7ae788 Compare November 13, 2023 16:25

short_description: login or logout to a registry.

version_added: "2.5.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
version_added: "2.5.0"
version_added: "3.0.0"

host: localhost:5000

# Logout from helm registry
- name: Logout to Remote registry
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- name: Logout to Remote registry
- name: Logout from Remote registry

if state == "absent" and "Error: not logged in" in err:
err = err.replace("Error: ", "")
changed = False
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the value of changed be set to False here?

chart_ref: "oci://localhost:{{ registry_port }}/helm-charts/python-app"
chart_version: 0.1.0
namespace: "{{ test_namespace[0] }}"
create_namespace: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we verify the success of this task to ensure that the login is, indeed, successful?

Copy link

username:
description:
- Provide a username for authenticating with the remote registry.
- Required when C(state) is set to I(present).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Required when C(state) is set to I(present).
- Required when I(state) is set to C(present).

password:
description:
- Provide a password for authenticating with the remote registry.
- Required when C(state) is set to I(present).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Required when C(state) is set to I(present).
- Required when I(state) is set to C(present).

options:
state:
description:
- If set to I(present) attempt to log in to the remote registry server using the URL specified in C(host).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- If set to I(present) attempt to log in to the remote registry server using the URL specified in C(host).
- If set to C(present) attempt to log in to the remote registry server using the URL specified in I(host).

state:
description:
- If set to I(present) attempt to log in to the remote registry server using the URL specified in C(host).
- If set to I(absent) attempt to log out by removing credentials stored for the remote registry server specified in C(host).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- If set to I(absent) attempt to log out by removing credentials stored for the remote registry server specified in C(host).
- If set to C(absent) attempt to log out by removing credentials stored for the remote registry server specified in I(host).

RETURN = r"""
stdout:
type: str
description: Full `helm` command stdout, in case you want to display it or examine the event log
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description: Full `helm` command stdout, in case you want to display it or examine the event log
description: Full `helm` command stdout, in case you want to display it or examine the event log.

sample: ''
stderr:
type: str
description: Full `helm` command stderr, in case you want to display it or examine the event log
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description: Full `helm` command stderr, in case you want to display it or examine the event log
description: Full `helm` command stderr, in case you want to display it or examine the event log.

Copy link
Contributor

@yurnov yurnov left a comment

Choose a reason for hiding this comment

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

Hi @abikouo,

would you please like to rebase and complete this PR?


short_description: login or logout to a registry.

version_added: "2.5.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

it should be 3.3.0 or even 5.1.0

@yurnov
Copy link
Contributor

yurnov commented Nov 21, 2024

Hi @abikouo

Disregard the previous comment, I have my own version that is tested in the production system (except the integration test part), I will sort out some formality and create PR in the beginning of next week.

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.

kubernetes.core.helm supports OCI registry
4 participants