Defaults ObservedGeneration to -1 and sets LastHandledReconcileAt

[luizbafilho]

closes #825 #3199

Defaults ObservedGeneration to -1 and sets LastHandledReconcileAt

[yiannistri]

@luizbafilho can you please squash some of the commits to better structure the changes? Thank you

[luizbafilho]

@luizbafilho can you please squash some of the commits to better structure the changes? Thank you

I generally just use

[squaremo]

I generally just use ["Squash and Merge" button]

In terms of keeping a reasonable git history, this is OK for small changes (and I think this PR is borderline). But in general it's not much help for reviewers and other readers of the code, who will often want to look at individual commits to see changes in isolation. It also means you, as the author, are not obliged to "show your working" (because it'll all get squashed together so whatever).

As a frequent reviewer I much prefer it when people use the commit history to explain their change. Have a look at https://github.com/weaveworks/engineering-handbook/blob/main/standards/git.md and https://github.com/weaveworks/engineering-handbook/blob/main/standards/code-review.md#preparing-a-pull-request.

[luizbafilho]

I generally just use ["Squash and Merge" button]

In terms of keeping a reasonable git history, this is OK for small changes (and I think this PR is borderline). But in general it's not much help for reviewers and other readers of the code, who will often want to look at individual commits to see changes in isolation. It also means you, as the author, are not obliged to "show your working" (because it'll all get squashed together so whatever).

As a frequent reviewer I much prefer it when people use the commit history to explain their change. Have a look at https://github.com/weaveworks/engineering-handbook/blob/main/standards/git.md and https://github.com/weaveworks/engineering-handbook/blob/main/standards/code-review.md#preparing-a-pull-request.

I just read those and I'm afraid I won't be able to adhere to much of the commits stuff, my development process is chaotic, I start at A, realize I need B, then jump to C, then delete B, and remember to make small changes and keeping those in self-contained commits adds too much cognitive load for my brain. If the PR is big enough that you need to walk through it commit by commit I think it needs to be multiple PRs instead.

[squaremo]

If the PR is big enough that you need to walk through it commit by commit I think it needs to be multiple PRs instead.

It's not necessarily the size of a PR, it can be the composition. Say you change the name of a method that's used in lots of places. To avoid obfuscating other changes, you would want to make that change in one commit and keep other changes to other commits. Then someone can discount the renaming when looking at the rest of them.

I start at A, realize I need B, then jump to C, then delete B, and remember to make small changes

This is how everyone works, to some extent. I often change my idea of how to do something a few commits in, and rewrite it. Then change my mind again, and so on.

The things is, software development is a team sport. Your job is not just to make the change, but to make the change in such a way that people can understand it -- when they review it, and when they turn up later to figure out why this code works like it does, or doesn't work. For the same reason you might write a comment explaining a piece of code for a future reader, you can use git to explain a change to a future reader. This is a crucial part of maintaining a high quality code base.

It's something you can learn and get better at. For here and now, I suggest

• squash 99772f6 into its predecessor (and explain it in the commit message -- why does it need the different package with the same name?)
• put flaky test marking up front (git rebase -i and move that line)
• make 13e7238 a fixup to f5b132b and aa0aa85. If this seems too fiddly, squash it with a6f4442 since it's the same change made to a few tests.

[squaremo]

Viewed in the whole, this looks correct and has tests demonstrating so: 💯

Most of my review comments are about git hygiene -- making an easy-to-follow trail for future code readers. I gave some suggestions for tidying up in a comment above the review, too. There's one code change which I think is necessary: please use the build tag mechanism to mark flaky tests.

I'm marking this as "request changes" so I can come back and review again. You don't have to follow all my suggestions to the letter, just consider them -- but please do change the flaky test thing :-)

[squaremo]

I presume this is a result of rerunning make generate or whatever after changing the go type ... in which case, it'd be worth mentioning these incidental changes in the commit message, so people don't have to wonder where they came from.

[squaremo]

You don't need to use Eventually here, the object should be created with the default value. Just fetch the object and check the status value.

[squaremo]

Assuming the e2e failure is a flake and will pass (I've kicked off a retry), this looks good to go. Thanks especially for tidying up the commits 👍

[squaremo]

👍