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

Spawn reload and reset actions to all pods #528

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dnlkoch
Copy link
Collaborator

@dnlkoch dnlkoch commented Sep 27, 2024

This suggests to emit the newly introduced LifecycleEvent events on reset (ResetEvent) and reload (ReloadEvent) respectively. Connected services will receive this events to actually reload and/or reset their catalog and configuration.

See #518.

@dnlkoch dnlkoch force-pushed the 518-spawn-reload-reset branch 4 times, most recently from ea41da3 to ee17fdd Compare September 30, 2024 09:34
@buehner
Copy link
Member

buehner commented Oct 2, 2024

@vuilleumierc @danduk82 We rebased against the main branch and it seems that the acceptance tests fail for some reason. But to me it looks this is not related to the changes in our branch, right?

@vuilleumierc
Copy link
Collaborator

@vuilleumierc @danduk82 We rebased against the main branch and it seems that the acceptance tests fail for some reason. But to me it looks this is not related to the changes in our branch, right?

Hi @buehner, I think you're right, it's not linked to your change. We will look at this Friday or next week, in the meantime feel free to ignore these two tests.

It seems the GetTile URL is wrong (http://gateway:8080/geoserver/cloud/geoserver/cloud/gwc/service/wmts), any chance this could be linked to your changes @pmauduit ? It could be that the GetCapabilities is adding 2 times /geoserver/cloud in the capabilities document

Copy link
Member

@groldan groldan left a comment

Choose a reason for hiding this comment

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

Hey this is looking good. I'm asking for a couple changes in the comments.
Additionally, please make sure to:

  • squash all the commits into a single one, with a meaningful title and an abstract explaining the changes.
  • check out for new issues introduced by the pull request as reported by SonarLint. You'll have to wait for the "SonarCloud QA" build job to finish.
    It currently reports 6 new issues:
    image

image

@groldan
Copy link
Member

groldan commented Oct 8, 2024

oh, btw, @buehner @dnlkoch, given this contribution is more than a couple lines, we need to make sure you've signed the GeoServer contribution agreement, either individually or covered by your company's CLA. Please check https://docs.geoserver.org/main/en/developer/policies/committing.html and the CONTRIBUTING.md file for instructions if not yet covered by a CLA.

@vuilleumierc
Copy link
Collaborator

Regarding the failing tests, they now pass on main (thanks @groldan) so if you rebase you should be all set

@buehner
Copy link
Member

buehner commented Oct 9, 2024

Thanks for the feedback. We will continue on this soon

@marcjansen
Copy link
Contributor

oh, btw, @buehner @dnlkoch, given this contribution is more than a couple lines, we need to make sure you've signed the GeoServer contribution agreement, either individually or covered by your company's CLA. Please check https://docs.geoserver.org/main/en/developer/policies/committing.html and the CONTRIBUTING.md file for instructions if not yet covered by a CLA.

CCLA sent just now to OSGeo, relevant people of the PR were in CC

@groldan
Copy link
Member

groldan commented Oct 14, 2024

Thanks @marcjansen , that's great.
@buehner @dnlkoch , I'm trying to wrap up 1.9.0 asap. Do you guys think you'd have time to work on the pr feedback this week?
Cheers

@buehner
Copy link
Member

buehner commented Oct 16, 2024

i think this week it will be rather difficult to find time to work on this, but on monday we should definitely have time for it

@groldan groldan added the enhancement New feature or request label Oct 23, 2024
Copy link
Member

@groldan groldan left a comment

Choose a reason for hiding this comment

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

@dnlkoch @buehner thanks a lot for addressing all the code reviews.
In my opinion, the only thing missing is to squash all 22 commits into a single one and provide a good title and description.
Sorry I'm picky about it, but having a bunch of commits with small comments for what's rather a single functionality is detrimental for the final log. Don't get me wrong, I do it all the time, and worse, I commit with just "tmp" or "WIP" comments, but once it's done I'd rather have a single or a couple of commits with sensible descriptions.

Other than that, congrats on the great work

@dnlkoch dnlkoch force-pushed the 518-spawn-reload-reset branch 4 times, most recently from e755881 to 1c8576f Compare October 30, 2024 11:18
@dnlkoch dnlkoch marked this pull request as ready for review October 30, 2024 12:13
@dnlkoch
Copy link
Collaborator Author

dnlkoch commented Oct 30, 2024

@groldan You're absolutely right! Having all of those interim commits contained in the history is surely not needed. Fixed it right now and thanks again for your valuable review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants