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

Testing patch v12.0.3 #3443

Merged
merged 9 commits into from
May 9, 2024
Merged

Conversation

laurenceisla
Copy link
Member

@laurenceisla laurenceisla commented Apr 24, 2024

Just testing some cherry-picks with minimal conflicts for a patch release.

The original strategy is to force push the changes in main to the v12 branch. Since only some commits are applied, I based it on the existing v12 branch instead.


From the current CHANGELOG, the included fixes are:

The missing fixes are:

@steve-chavez
Copy link
Member

Even with just some fixes, a new patch release LGTM. If the others are hard to merge, we can leave them for the next minor.

@wolfgangwalther
Copy link
Member

I don't understand why we want a new patch release right now. We agreed in #3113 to work on the next minor release immediately. The docs for the features added before the docs-migration are still missing, though. Instead, we're getting new features worked on all the time. What am I missing?

@wolfgangwalther
Copy link
Member

The original strategy is to force push the changes in main to the v12 branch. Since only some commits are applied, I based it on the existing v12 branch instead.

I don't understand what that means. You are proposing to merge this into v12, which was exactly the plan, right? What am I missing?


I'd still need to finish the release workflow to make it possible to release on the v12 branch properly. I haven't come around to finishing this, yet.

@laurenceisla
Copy link
Member Author

I don't understand what that means. You are proposing to merge this into v12, which was exactly the plan, right? What am I missing?

No, no, there shouldn't be a problem. Mostly mentioned it because I didn't know if the release workflow may be different between force pushing the main branch and forking and merging the v12 one.

@wolfgangwalther
Copy link
Member

Mostly mentioned it because I didn't know if the release workflow may be different between force pushing the main branch and forking and merging the v12 one.

I'm not sure what you mean by force pushing the main branch - but I don't ever want to do that. Maybe there is a mis-understanding. I imagine the future development flow for back-branches to be basically exactly how you did it here: Cherry-pick bugfixes. But then do the cherry-picking + PR semi-automatically and also auto-release bugfix releases after commits to the back-branches, similar to nightly releases on the main branch.

@laurenceisla
Copy link
Member Author

I'm not sure what you mean by force pushing the main branch - but I don't ever want to do that. Maybe there is a mis-understanding.

Ah cool, I get it now. I misunderstood here: #2814 (comment), I now notice that you never mentioned the main branch.

But then do the cherry-picking + PR semi-automatically and also auto-release bugfix releases after commits to the back-branches, similar to nightly releases on the main branch.

Got it.

@wolfgangwalther
Copy link
Member

Ah cool, I get it now. I misunderstood here: #2814 (comment), I now notice that you never mentioned the main branch.

Ah, I had forgotten about this. Yeah, we might need to still force push the release branch (v12) after a new minor release. But this should not affect the patch release workflow.

steve-chavez and others added 9 commits May 9, 2024 18:08
415 is for Content-Type and 406 for Accept headers.
…are used

Previously using a generic mimetype handler failed when any kind of select= was given, because
we tried to cast the select-result to the original table type. With this change, this cast is
only applied when select=* is given implicitly or explicitly. This is the only case where this
makes sense, because this guarantees that correct columns are selected in the correct order for
this cast to succeed.

Resolves PostgREST#3160
@wolfgangwalther
Copy link
Member

I rebased this on latest v12, then ran postgrest-release for this branch (technically needs to run on the branch named v12, so did that locally) and added the resulting bump commit here.

We can merge this into v12 when pipelines pass - this should then kick off the new release, including tag etc. Let's see whether that works.

@wolfgangwalther wolfgangwalther marked this pull request as ready for review May 9, 2024 16:11
@wolfgangwalther wolfgangwalther merged commit 1afd98a into PostgREST:v12 May 9, 2024
34 checks passed
@laurenceisla laurenceisla deleted the launch-v12.0.3 branch May 9, 2024 17:01
@laurenceisla
Copy link
Member Author

laurenceisla commented May 9, 2024

@wolfgangwalther Great job on the new release workflow, it's looking great!

We can merge this into v12 when pipelines pass - this should then kick off the new release, including tag etc. Let's see whether that works.

Some feedback here:

@wolfgangwalther
Copy link
Member

The release was generated https://github.com/PostgREST/postgrest/releases/tag/v12.0.3, but the content from the CHANGELOG with the list of Fixes didn't show (I added them manually).

Haha, I was heavily confused when I just got a notification for the release without any notes - and then clicked the release and all notes were there... ok I will investigate.

The Docker release for v12.0.3 does not include the arm64 image. According to the error in the GH Actions, it's using vv12.0.3 as tag, so it mustn't add a v anymore here:

Argh. I only introduced that conditional logic because I had vdevel tags on main with the exact same result... turns out I should have just removed the v prefix entirely...

@wolfgangwalther
Copy link
Member

According to the error in the GH Actions,

I wonder why we have an error here... but the job still passes. Seems like there is no error code returned.

@wolfgangwalther
Copy link
Member

Ok, I fixed all three issues. To get the arm docker image properly build and pushed, I will need to re-run the whole release pipeline - to not make it fail, I will delete the github release soon, it will then be recreated again when I force push the v12.0.3 tag to after my fixes.

@wolfgangwalther
Copy link
Member

While fixing the error codes for the arm scripts.. I realized that this was broken ever since 83cf15f, which was supposed to pass a GHC_VERSION argument. But GHC_VERSION is not $GHC_VERSION - and thus ghcup was always throwing errors when trying to install that literal GHC_VERSION string. But nobody noticed, because those scripts weren't set -e.. :/

Let's see what that means. Hopefully things will just build fine when we now pass the proper version to it.

@wolfgangwalther
Copy link
Member

Hm, the latest push for the tag failed with a "no space left" error for the arm job: https://github.com/PostgREST/postgrest/actions/runs/9022807863/job/24793212610

This might be because too many arm jobs were running at the same time. Just to be sure, @laurenceisla, can you check on the runner again whether we might have left-over build folders again which were not cleaned up?

I will wait a bit until all other jobs have settled down and then run it again.

@wolfgangwalther
Copy link
Member

This might be because too many arm jobs were running at the same time. Just to be sure, @laurenceisla, can you check on the runner again whether we might have left-over build folders again which were not cleaned up?

If you happen to have a chance to look into this before I do again tomorrow, you can also restart the failed jobs in this pipeline: https://github.com/PostgREST/postgrest/actions/runs/9022807863/job/24793212610 - this should then continue the release pipeline. I'll look into it tomorrow again.

@laurenceisla
Copy link
Member Author

laurenceisla commented May 9, 2024

But GHC_VERSION is not $GHC_VERSION - and thus ghcup was always throwing errors when trying to install that literal GHC_VERSION string.

Ooph, nice catch.

If you happen to have a chance to look into this before I do again tomorrow, you can also restart the failed jobs in this pipeline: https://github.com/PostgREST/postgrest/actions/runs/9022807863/job/24793212610 - this should then continue the release pipeline. I'll look into it tomorrow again.

That's strange, the server doesn't seem to have older build folders there... maybe with the GHC fix, it added more libraries to the .cabal folder. I'll check it out and will restart when I find the culprit.

@laurenceisla
Copy link
Member Author

laurenceisla commented May 9, 2024

It was GHCup itself that had GBs of tmp files while installing, I think both CI jobs for main and v12.0.3 collided in installing their GHC versions. Anyways, I nuked the whole ghcup/cabal and reinstalled again. I'm also restarting the jobs with the one in main first.

@laurenceisla
Copy link
Member Author

All done! it's working correctly now 🎉

Confirmed that both of the issues mentioned here are fixed #3443 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants