odpi / egeria-charts

Helm chart repository
https://odpi.github.io/egeria-charts
Apache License 2.0
13 stars 9 forks source link

Various changes and improvements #236

Closed T4ylan closed 1 year ago

T4ylan commented 1 year ago

Changes

All changes are passive, as in they default to false, so they are backward compatible.

planetf1 commented 1 year ago

Thanks for a great list of improvements.

I went to review, but noticed some oddities with the delta -- the existing code in main is already at 3.15 - and there's a list of commits included early on that are already in main. I think some rebuilding of the PR may be needed to fixup the history?

I think this can happen when using squash merges? Or anytime the history is rebuilt, since the commit ids representing changes may have diverged from main

Can you describe the process? Maybe we can figure out how it happened, and how to recommend to others when working on a PR to avoid it happening

it causes difficulties in reviewing. I'm unsure if it will break the commit history - part of the issue may be github. However when this has happened before (only version occasionally, though most recently with spring) we've had to rebuild the PR (it new base, cherry-pick fixes across) so that we can be comfortable with the review

We have 4.0 going out soon - I think we'll likely need to merge/fix #211 first so that the UI can be fixed. It does change a few things around the UI specifically but not much else.

But if we can get this in for 4.0 also that would be great (but a 4.0.1 is fine too)

I'll review once we've sorted the history. I can help do that if needed

T4ylan commented 1 year ago

Thanks for a great list of improvements.

I went to review, but noticed some oddities with the delta -- the existing code in main is already at 3.15 - and there's a list of commits included early on that are already in main. I think some rebuilding of the PR may be needed to fixup the history?

I think this can happen when using squash merges? Or anytime the history is rebuilt, since the commit ids representing changes may have diverged from main

Can you describe the process? Maybe we can figure out how it happened, and how to recommend to others when working on a PR to avoid it happening

it causes difficulties in reviewing. I'm unsure if it will break the commit history - part of the issue may be github. However when this has happened before (only version occasionally, though most recently with spring) we've had to rebuild the PR (it new base, cherry-pick fixes across) so that we can be comfortable with the review

We have 4.0 going out soon - I think we'll likely need to merge/fix #211 first so that the UI can be fixed. It does change a few things around the UI specifically but not much else.

But if we can get this in for 4.0 also that would be great (but a 4.0.1 is fine too)

I'll review once we've sorted the history. I can help do that if needed

Yes, that was definitely my fault. When I started, the version was on 3.14 (I believe), then came the 3.15-prerelease, etc.. Since I was still developing now and then, I just added the changes on the main to our branch. That's why it looks like this. Nothing from the main should be missing though.

I can do anything to make the review process easier. Please just let me know. About the release, I'd be more than happy if any of these changes gets into the main, when doesn't really matter imho.

Taylan

Edit: I'm not proud of my commit history in the branch, so it could go away if you ask me.

planetf1 commented 1 year ago

It can cause problems with diffs in github, especially review process.

What I've done in the past is to take a clean base and cherry-pick the changes. We had a similar problem with spring - as it was a long running change. In that case there was some complexity in conflicts requiring some careful review, but here it should be much simpler as there's been less activity in the charts

Rebasing may also help

I can take a copy of your branch, and see if anything simple can help. If not we could rebuild commit by commit. I can do this for you if it helps (but will preserve the commits)

I really need to do this before I review..

I'll assume I will give this a go... and I'll mention you on any new review.

I'll do this if I don't hear, but I have another urgent change for 4 I need to complete first

I would really like to get the changes in too - really appreciate the hard work you put into the improvements.

planetf1 commented 1 year ago

(moving to draft whilst we figure out a rebuild)

planetf1 commented 1 year ago

This is one way of cleaning up the commits

I did notice one less file was modified. I've not checked closely....

I've done this as an example and posted to #237. I can review this. (but it does result in my signup being added to yours in the commits) But if you would like to try this process and check you're happy with it?

T4ylan commented 1 year ago

I've done this as an example and posted to #237. I can review this. (but it does result in my signup being added to yours in the commits) But if you would like to try this process and check you're happy with it?

I'm more than happy to try myself, so I don't botch up next time (and I don't mind, if your signup is being added to these commits). Sadly I won't be able to do much this week, for reasons discussed earlier in slack.