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 recommended labels #1705

Merged
merged 4 commits into from
Mar 26, 2024
Merged

Add recommended labels #1705

merged 4 commits into from
Mar 26, 2024

Conversation

xoanmi
Copy link
Contributor

@xoanmi xoanmi commented Feb 13, 2024

Signed-off-by: Joan Miquel Luque Oliver <[email protected]>
@Vad1mo
Copy link
Member

Vad1mo commented Feb 14, 2024

This is also related to #1471

Copy link
Member

@Vad1mo Vad1mo left a comment

Choose a reason for hiding this comment

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

Did you try and verify the upgrade backwards compatibility?

@xoanmi
Copy link
Contributor Author

xoanmi commented Feb 14, 2024

Did you try and verify the upgrade backwards compatibility?

Good catch! I've removed the new labels from the matchSelector as it is immutable. Now the change is backwards compatible 🎉

Install latest

➜ helm install harbor harbor/harbor \
    --set externalURL=http://core.harbor.domain \
    --set expose.ingress.hosts.core=core.harbor.domain \
    --set persistence.enabled=false
NAME: harbor
LAST DEPLOYED: Wed Feb 14 12:45:49 2024
NAMESPACE: harbor
STATUS: deployed
REVISION: 1
TEST SUITE: None
NOTES:
Please wait for several minutes for Harbor deployment to complete.
Then you should be able to visit the Harbor portal at http://core.harbor.domain
For more details, please visit https://github.com/goharbor/harbor

Upgrade from release

➜ helm upgrade harbor . \
    --set externalURL=http://core.harbor.domain \
    --set expose.ingress.hosts.core=core.harbor.domain \
    --set persistence.enabled=false
Release "harbor" has been upgraded. Happy Helming!
NAME: harbor
LAST DEPLOYED: Wed Feb 14 12:55:45 2024
NAMESPACE: harbor
STATUS: deployed
REVISION: 4
TEST SUITE: None
NOTES:
Please wait for several minutes for Harbor deployment to complete.
Then you should be able to visit the Harbor portal at http://core.harbor.domain
For more details, please visit https://github.com/goharbor/harbor

@xoanmi xoanmi requested a review from Vad1mo February 14, 2024 12:12
@xoanmi
Copy link
Contributor Author

xoanmi commented Feb 21, 2024

@Vad1mo could you please review this PR? I think it's a very innocuous change and could be merged without further discussion, isn't it?

@Vad1mo
Copy link
Member

Vad1mo commented Feb 21, 2024

I am all in here, need to test it thou first to see the backward compatibility.

@zyyw
Copy link
Collaborator

zyyw commented Feb 22, 2024

Do we have a strong reason why we need to add the Standard labels, instead of using the current labels. Is there any project that also add the Standard labels in addition to the their current ones?

@xoanmi
Copy link
Contributor Author

xoanmi commented Feb 22, 2024

Do we have a strong reason why we need to add the Standard labels, instead of using the current labels. Is there any project that also add the Standard labels in addition to the their current ones?

In general, all projects use the standard labels. I didn't delete the custom ones to not break retro compatibility.

I see it the other way around: Is there any compelling reason not to use the recommended labels? Labels are for free and there is no risk to break anything so why don't do it?

@xoanmi
Copy link
Contributor Author

xoanmi commented Feb 22, 2024

In addition, we have in place some processes that require the presence of those standard labels in all the deployments. That's our main reason to request adding them here :)

@xoanmi xoanmi requested a review from cvegagimenez February 22, 2024 09:08
Copy link
Collaborator

@zyyw zyyw left a comment

Choose a reason for hiding this comment

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

Thank you for contributing to harbor-helm project!

@xoanmi
Copy link
Contributor Author

xoanmi commented Mar 26, 2024

Thank you for contributing to harbor-helm project!

@zyyw could we merge this PR?

@Vad1mo Vad1mo merged commit f05cca7 into goharbor:main Mar 26, 2024
6 checks passed
@cvegagimenez
Copy link
Contributor

Hi @zyyw ,

do you know when this change will be added to a new release?

@zyyw
Copy link
Collaborator

zyyw commented Apr 18, 2024

Hi @zyyw ,

do you know when this change will be added to a new release?

If everything goes well, it should be available in the 2nd half of May or early of June.

@zyyw
Copy link
Collaborator

zyyw commented Apr 24, 2024

FYI, if we set persistence.enabled=true, the upgrading process will be failed as mentioned in this issue:

Created a PR to resolve this upgrading issue:

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.

Missing recommended labels
4 participants