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

Merge release/v1.14 in to release/v2.0 #3862

Closed
wants to merge 200 commits into from

Conversation

LukeShu
Copy link
Contributor

@LukeShu LukeShu commented Oct 5, 2021

Description

Things have happened in release/v1.14 that need to get merged over to release/v2.0. Some of this work has already been manually re-done. Some of it has been cherry-picked. So, figuring out what needs to be merged/copied over is difficult.

The simplified version of how I did it:

  1. Rebase release/v1.14 on to release/v2.0 (rather than doing a simple merge), so that I can address conflicts commit by commit instead of all at once.
  2. After I've done the rebase, git merge -s ours origin/release/v1.14 to tell Git what we've done; so we don't need to look at those changes again in the future when we want to pull new future changes from v1.14 in to v2.0.

When I started all this:

Here's the full version of how I did it:

  • Prep to make the rebase easier: First I found the point at which release/v1.14 and release/v2.0 diverged, and I rebased release/v1.14 on to that fork-point, in order to linearize the history. I've pushed this as the lukeshu/1.14-merge/before branch.
  • Prep to make the rebase easier: I created a lukeshu/base branch that was the merge of release/v2.0 and master (at the time, neither was fully merged in to the other), and also had 2 more commits added to it that I thought would make the rebase easier. I haven't pushed this branch separately, but it is 9c2cbab
  • I rebased lukeshu/1.14-merge/before on to that lukeshu/base branch (with assistance and guidance from @kflynn); I've pushed this as lukeshu/1.14-merge/after.
  • By this point, master, release/v2.0, and release/v1.14 had all had new stuff on them. so I "re-did" lukeshu/base as [2.0] Misc fixups noticed when looking at the 1.14 merge #3860, and rebased lukeshu/1.14-merge/after on to it (I named this new branch lukeshu/1.14-merge-2/after)
  • I then did git merge -s ours 4294fbe6528af6c37cf8332f1437186b7a5c73ce; since that is the commit that release/v1.14 was at when I started.

Notes about my rebase methodology:

  • I made sure that each commit was make generate-clean; often times this forced me to edit CHANGELOG.tpl so that it wouldn't cause a change in CHANGELOG.md
  • Whenever I saw conflicts in generated files (other than CHANGELOG.md, since it isn't generated on release/v1.14), I'd resolve the conflicts in the non-generated files, delete all of the generated files, then run make generate`. The delete step was important because v1.14 has some generated files that v2.0 doesn't, so deleting everything made sure I didn't accidentally re-add anything that I shouldn't have.
  • Whenever there were conflicts because pkg/api/getambassador.io/v2/ has been deleted in 2.0, I looked at the original commit, and re-did those same changes in pkg/api/getambassador.io/v3alpha1/.

Put another way, this is #3859, but

  1. Fixed
  2. Rebased on to [2.0] Misc fixups noticed when looking at the 1.14 merge #3860
  3. then git merge -s ours ${the commit that release/v1.14 was at when we started}

To view the commits in this, you'll probably want to start at the penultimate commit (origin/lukeshu/1.14-merge-2/after^); the ultimate commit is just a boring merge -s ours; no files actually change in it.

Related Issues

List related issues.

Testing

A few sentences describing what testing you've done, e.g., manual tests, automated tests, deployed in production, etc.

Checklist

  • I made sure to update CHANGELOG.md.

    Remember, the CHANGELOG needs to mention:

    • Any new features
    • Any changes to our included version of Envoy
    • Any non-backward-compatible changes
    • Any deprecations
  • This is unlikely to impact how Ambassador performs at scale.

    Remember, things that might have an impact at scale include:

    • Any significant changes in memory use that might require adjusting the memory limits
    • Any significant changes in CPU use that might require adjusting the CPU limits
    • Anything that might change how many replicas users should use
    • Changes that impact data-plane latency/scalability
  • My change is adequately tested.

    Remember when considering testing:

    • LEGACY MODE TESTS DO NOT RUN FOR EVERY PR. If your change is affected by legacy mode, you need
      to run legacy-mode tests manually (set AMBASSADOR_LEGACY_MODE=true and run the tests).
      (This will be fixed soon.)
    • Your change needs to be specifically covered by tests.
      • Tests need to cover all the states where your change is relevant: for example, if you add a behavior that can be enabled or disabled, you'll need tests that cover the enabled case and tests that cover the disabled case. It's not sufficient just to test with the behavior enabled.
    • You also need to make sure that the entire area being changed has adequate test coverage.
      • If existing tests don't actually cover the entire area being changed, add tests.
      • This applies even for aspects of the area that you're not changing – check the test coverage, and improve it if needed!
    • We should lean on the bulk of code being covered by unit tests, but...
    • ... an end-to-end test should cover the integration points
  • I updated DEVELOPING.md with any any special dev tricks I had to use to work on this code efficiently.

acookin and others added 30 commits June 11, 2021 16:49
Signed-off-by: Alix Cook <[email protected]>
Signed-off-by: Alix Cook <[email protected]>
Signed-off-by: Alix Cook <[email protected]>
* run envoy in docker so we can run it anywhere

Signed-off-by: Alix Cook <[email protected]>

* more make dependencies

Signed-off-by: Alix Cook <[email protected]>

* bump kat request limit

Signed-off-by: Alix Cook <[email protected]>

* add commit hook for dco bot

Signed-off-by: Alix Cook <[email protected]>

* do docker builds better

Signed-off-by: Alix Cook <[email protected]>

* hmm maybe dont wait on oss images

Signed-off-by: Alix Cook <[email protected]>
* fix for wait for rc artifacts

Signed-off-by: Alix Cook <[email protected]>

* change chart repo in a couple places

Signed-off-by: Alix Cook <[email protected]>
Signed-off-by: Flynn <[email protected]>
Signed-off-by: Flynn <[email protected]>
* cors mapping fix

Signed-off-by: Alix Cook <[email protected]>

* fake test

Signed-off-by: Alix Cook <[email protected]>

* changelog

Signed-off-by: Alix Cook <[email protected]>
Signed-off-by: Aidan Hahn <[email protected]>
Signed-off-by: Aidan Hahn <[email protected]>
Signed-off-by: Flynn <[email protected]>
…itch

[1.13.10] Allow disabling Ambex ratelimiter
Flynn and others added 19 commits October 4, 2021 19:45
…le metrics sink service.

Signed-off-by: Rafael Schloming <[email protected]>

Signed-off-by: Luke Shumaker <[email protected]>
Signed-off-by: AliceProxy <[email protected]>

Signed-off-by: Luke Shumaker <[email protected]>
Signed-off-by: AliceProxy <[email protected]>

Signed-off-by: Luke Shumaker <[email protected]>
* gzip fix

* update ambex

* update ambex

* update circle
Signed-off-by: Luke Shumaker <[email protected]>

Signed-off-by: Alix Cook <[email protected]>
Signed-off-by: Aidan Hahn <[email protected]>

Signed-off-by: Luke Shumaker <[email protected]>
Signed-off-by: Aidan Hahn <[email protected]>

Signed-off-by: Luke Shumaker <[email protected]>
Signed-off-by: Luke Shumaker <[email protected]>

Signed-off-by: Aidan Hahn <[email protected]>
Signed-off-by: Aidan Hahn <[email protected]>

Signed-off-by: Luke Shumaker <[email protected]>
* update aes version in chart

Signed-off-by: Alix Cook <[email protected]>

* update yaml

Signed-off-by: Alix Cook <[email protected]>
Signed-off-by: Luke Shumaker <[email protected]>
Signed-off-by: Aidan Hahn <[email protected]>

Signed-off-by: Luke Shumaker <[email protected]>
Signed-off-by: AliceProxy <[email protected]>

Signed-off-by: Luke Shumaker <[email protected]>
Signed-off-by: AliceProxy <[email protected]>

Signed-off-by: Luke Shumaker <[email protected]>
Signed-off-by: AliceProxy <[email protected]>

Signed-off-by: Luke Shumaker <[email protected]>
Signed-off-by: AliceProxy <[email protected]>

Signed-off-by: Luke Shumaker <[email protected]>
Signed-off-by: Flynn <[email protected]>

Signed-off-by: Luke Shumaker <[email protected]>
Signed-off-by: AliceProxy <[email protected]>

Signed-off-by: Luke Shumaker <[email protected]>
Signed-off-by: AliceProxy <[email protected]>

Signed-off-by: Luke Shumaker <[email protected]>
@LukeShu LukeShu force-pushed the lukeshu/1.14-merge-2/after branch from e09f6a7 to 9bbbe0b Compare October 5, 2021 01:45
Copy link
Member

@kflynn kflynn left a comment

Choose a reason for hiding this comment

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

This PR looks really good! I found one weirdness in the chart, and one other thing: comparing this to #3859 (with git difftool db0cef224011f1fff430d03e89ebc7512857a628..9bbbe0be86bdeac2a135b87f3908578a0ccffb5c) turns up some weird-looking diffs in python/schemas/v2/Mapping.schema, but overall looks like just what we want. Lemme get your take on that diff before we land it...

Comment on lines +37 to +40
## v6.9.1

- Update Ambassador API Gateway chart image to version v1.14.1: [CHANGELOG](https://github.com/emissary-ingress/emissary/blob/master/CHANGELOG.md)
- Update Ambassador Edge Stack chart image to version v1.14.1: [CHANGELOG](https://github.com/emissary-ingress/emissary/blob/master/CHANGELOG.md)
Copy link
Member

Choose a reason for hiding this comment

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

We did a v6.9.2 for 1.14.2 -- just realized that we didn't see that when we were talking today. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't out yet when we started, so it's not included. There'll have to be a later (much easier!) merge to pull that in.

Actually, if CI passes on your fixes, I'll go ahead and include that in this PR.

@LukeShu
Copy link
Contributor Author

LukeShu commented Oct 5, 2021

comparing this to #3859 (with git difftool db0cef224011f1fff430d03e89ebc7512857a628..9bbbe0be86bdeac2a135b87f3908578a0ccffb5c) turns up some weird-looking diffs in python/schemas/v2/Mapping.schema,

That looks to me like make format's doing. I commented on your PR that the lint check was failing because the file wasn't formatted with make format.

@LukeShu
Copy link
Contributor Author

LukeShu commented Oct 5, 2021

Actually, I'm not so sure about it just being make format... I'll look closer in the morning.

Base automatically changed from lukeshu/2.0-fixup to release/v2.0 October 5, 2021 22:03
@LukeShu
Copy link
Contributor Author

LukeShu commented Oct 7, 2021

Closing in favor of #3867

@LukeShu LukeShu closed this Oct 7, 2021
@LukeShu LukeShu deleted the lukeshu/1.14-merge-2/after branch November 9, 2021 23:29
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.

6 participants