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

Support for ingress controller #1668

Merged
merged 6 commits into from
Jun 29, 2023
Merged

Support for ingress controller #1668

merged 6 commits into from
Jun 29, 2023

Conversation

vbricentic
Copy link

Support for ingress controller
Creation of ingress-nginx namespace

@thjaeckle
Copy link
Member

Thanks @vbricentic for this contribution.
Could you please sign your commit with the e-mail-address registered to your Eclipse account (and where you have signed the ECA)?
See also CONTRIBUTING.md

@vbricentic
Copy link
Author

Thanks for the suggestion @thjaeckle . I have just signed the commit and eclipsefdn/eca. Is there a way to re-trigger the check or I should maybe recreate PR?

@thjaeckle
Copy link
Member

Thanks for the suggestion @thjaeckle . I have just signed the commit and eclipsefdn/eca. Is there a way to re-trigger the check or I should maybe recreate PR?

It seems something is still off .. see the Eclipse ECA tool:
image

Did you configure your git email-address correctly to the one used in your eclipse account?

In addition, other checks fail as well:

  • linting of the chart (the chart version needs to be increased - see the "Details")
  • license headers are missing from files (see the "Details")

@vbricentic
Copy link
Author

It should be better now, but not sure if everything is now solved. @thjaeckle could you please re-trigger check again.

@brvbosch
Copy link
Contributor

Account issue finally resolved.

@thjaeckle I'm little bit confused with chart versioning. Lint and test Helm chart requires to change. In code it says it's effectively set by release-job. Version of helm currently matches application version, so I tried with 3.3.0.1 but the format was rejected. I set now to 3.3.1 but might be misleading.

@thjaeckle
Copy link
Member

thjaeckle commented Jun 27, 2023

@vbricentic The chart version is still invalid:

Error: validation: chart.metadata.version "3.3.0.1" is invalid

Version of helm currently matches application version, so I tried with 3.3.0.1 but the format was rejected. I set now to 3.3.1 but might be misleading.

Yes, unfortunately we cannot always keep the versions in sync.
The Helm chart version will sometimes be ahead of the application version.

It is ok to change to 3.3.1

@brvbosch
Copy link
Contributor

brvbosch commented Jun 27, 2023

@thjaeckle in that case it should be now fine, I set it to 3.3.2 to not conflict with #1667
So it would just need to merge after that

Copy link
Member

@thjaeckle thjaeckle left a comment

Choose a reason for hiding this comment

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

Hey @vbricentic
I did a first round of reviews and added some comments.

Please move the ingress "controller" specific config to a separate YAML "object" - and make it configurable that the Ingress controller is deployed or not (if Ingress is enabled).

deployment/helm/ditto/templates/nginx-ingress.yaml Outdated Show resolved Hide resolved
deployment/helm/ditto/templates/nginx-ingress.yaml Outdated Show resolved Hide resolved
deployment/helm/ditto/templates/nginx-ingress.yaml Outdated Show resolved Hide resolved
deployment/helm/ditto/templates/nginx-ingress.yaml Outdated Show resolved Hide resolved
deployment/helm/ditto/values.yaml Outdated Show resolved Hide resolved
deployment/helm/ditto/values.yaml Outdated Show resolved Hide resolved
deployment/helm/ditto/values.yaml Outdated Show resolved Hide resolved
@brvbosch
Copy link
Contributor

brvbosch commented Jun 29, 2023

@thjaeckle version bumped again

@thjaeckle thjaeckle added this to the 3.3.1 milestone Jun 29, 2023
Copy link
Member

@thjaeckle thjaeckle left a comment

Choose a reason for hiding this comment

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

@vbricentic awesome, thanks for the adjustments and for the contribution. 👍
I will add it to the "3.3.1" milestone and will release it with a Helm chart version containing the Ditto 3.3.1 bugfix release.

@thjaeckle thjaeckle merged commit b264783 into eclipse-ditto:master Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants