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

update ingress snippet comment in values.yaml #241

Merged
merged 3 commits into from
Apr 16, 2023

Conversation

Northcode
Copy link
Contributor

Pull Request

Description of the change

Update well-known webfinger and nodeinfo redirects to match documentation in https://docs.nextcloud.com/server/23/admin_manual/issues/general_troubleshooting.html#service-discovery

Benefits

People just uncommenting the snippet will now have correct webfinger config, and no warnings in the nextcloud settings.

Possible drawbacks

None that I can see

Applicable issues

#240

Additional information

...

Checklist

@jessebot
Copy link
Collaborator

Could you please update the chart version and also link the most recent version of the documentation you're referencing? Other than that, it looks okay to me. Thanks for your contribution! :)

charts/nextcloud/Chart.yaml Outdated Show resolved Hide resolved
charts/nextcloud/values.yaml Outdated Show resolved Hide resolved
@Jeroen0494
Copy link

Jeroen0494 commented Mar 1, 2023

@Northcode any chance you could revisit this PR? I can confirm this fix works.

@Northcode
Copy link
Contributor Author

@Jeroen0494
Hi. Sorry for the late reply. I had forgotten about this.
I've fixed the things jessebot was complaining about and rebased on upstream.
Not sure how to proceed as github still says jessebot is requesting changes.

@Northcode Northcode requested a review from jessebot March 26, 2023 21:08
@tvories
Copy link
Collaborator

tvories commented Mar 27, 2023

@Northcode

I think @jessebot was requesting you remove the extra line between 34 and 35 in values.yaml

@Northcode
Copy link
Contributor Author

@tvories
I think it was complaining that there was trailing whitespace on line 35 of the old commit.
I've corrected that in my updated code, but it doesn't look like jessebot has rerun, its still complaining about the old commit.
I marked those corrections are resolved, is there something else I have to do for it to detect my changes?

@Jeroen0494
Copy link

I marked those corrections are resolved, is there something else I have to do for it to detect my changes?

You can go to the "1 change requested" option, click the menu and select "Request re-review" or something.
image

@jessebot
Copy link
Collaborator

I think it was complaining that there was trailing whitespace on line 35 of the old commit.
I've corrected that in my updated code, but it doesn't look like jessebot has rerun, its still complaining about the old commit.

Also, really quick, I'm a real person. I can't be rerun because I'm not a bot 😂 My username just has the word bot in it :)

@Northcode Northcode force-pushed the patch-1 branch 2 times, most recently from 04b0f44 to 098f6dc Compare April 11, 2023 20:16
Update well-known webfinger and nodeinfo redirects to match documentation in https://docs.nextcloud.com/server/23/admin_manual/issues/general_troubleshooting.html#service-discovery

Signed-off-by: Andreas Larsen <[email protected]>
@Northcode
Copy link
Contributor Author

@jessebot oh I see. In that case sorry for assuming your uhm.. type of consciousness? I guess? haha

Anyways I've removed the whitespace now on both line 36 and 33 and rebased onto upstream again, incrementing the chart version to 3.5.8

Copy link
Collaborator

@jessebot jessebot left a comment

Choose a reason for hiding this comment

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

chart.yaml needs bumping. Left the suggestion so you can just click the button and we can merge this. Thank you for fixing the whitespace :)

charts/nextcloud/Chart.yaml Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants