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

Unified Annotation Handling without Duplicates #1821

Open
wants to merge 6 commits into
base: devel
Choose a base branch
from

Conversation

alexiseichst
Copy link

@alexiseichst alexiseichst commented Apr 11, 2024

The objective here is to manage multiple entries and unify web_annotations and annotations while avoiding duplicates.

ISSUE TYPE
  • Bug, Docs Fix or other nominal change

@@ -44,11 +44,14 @@ spec:
] %}
checksum-secret-{{ secret }}: "{{ lookup('ansible.builtin.vars', secret, default='')["resources"][0]["data"] | default('') | sha1 }}"
{% endfor %}
{% if web_annotations %}
{{ web_annotations | indent(width=8) }}

Choose a reason for hiding this comment

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

This case was only accepting one single annotation, In our case we need multiple lines

{% if web_annotations %}
{{ web_annotations | indent(width=8) }}
{% elif annotations %}
{{ annotations | indent(width=8) }}

Choose a reason for hiding this comment

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

even worse that 'elif' case was completely overided by web_annotation...

Copy link

@RobinSegura RobinSegura left a comment

Choose a reason for hiding this comment

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

Nicely done @alexiseichst !

{% endif %}
{% set combined_annotations = web_annotations + annotations %}
{% set seen = [] %}
{% for annotation in combined_annotations %}

Choose a reason for hiding this comment

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

very elegant way to factorize code with minimum impact

@kurokobo
Copy link
Contributor

Could you please clarify your first issue, and provide actual AWX CR that can reproduce your issue? How you've tested this PR?
Also, is there any specific reason why you don't make any changes for task_annotations?

alexiseichst and others added 3 commits April 12, 2024 09:28
Typo fix

Update web.yaml.j2

Auteur :     alexiseichst <[email protected]>
Date :       Thu Apr 11 15:53:19 2024 +0200
Ajout du task et facto de tes commit
Copy link
Member

@rooftopcellist rooftopcellist left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @alexiseichst

My only note is that it might be nice to have a note in the docs here about how the annotations will now be merged:

And also add entries for task_annotations and web_annotations in the doc.

Feel free to add this in a follow-up PR if you agree.

Copy link
Member

@rooftopcellist rooftopcellist left a comment

Choose a reason for hiding this comment

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

These changes should be done for the task.yaml.j2 file as well.

@rooftopcellist
Copy link
Member

@alexiseichst I think we should add these same changes to the task deployment before merging. And CI is currently failing. This PR needs more attention before it can be merged.

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.

4 participants