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.

Sign up for GitHub

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

Jump to bottom

Fix make clobber, and have CI check that it works #4238

Merged
merged 10 commits into from
May 20, 2022
Merged

Fix make clobber, and have CI check that it works #4238

merged 10 commits into from
May 20, 2022

Conversation

LukeShu
Copy link
Contributor

@LukeShu LukeShu commented May 19, 2022 •
edited
Loading

Description

It isn't fixed if CI isn't checking it!

Also, a hack to py-list-deps to get it to work with newer setuptools, since my laptop has newer setuptools.

Related Issues

List related issues.

Testing

I had CI check that make clobber removes things after each and every CI job.

It's possible that I made a mistake with one of the CI jobs that doesn't run on PRs.

Checklist

  • I made sure to update CHANGELOG.md. - no applicable changes

    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. - not a runtime change

    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:

    • 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. - no tricks

  • The changes in this PR have been reviewed for security concerns and adherence to security best practices.

@LukeShu LukeShu mentioned this pull request May 19, 2022
Closed
5 tasks
@LukeShu LukeShu force-pushed the lukeshu/clobber branch 4 times, most recently from caf2237 to fc9e910 Compare May 19, 2022 09:11
LukeShu added 2 commits May 19, 2022 11:11
@LukeShu
.github: Clean up the log collection
a708885 
 - Rename `actions/collect-testing-logs` -> `actions/collect-logs`
 - Rename the `name` arg to `jobname`
 - Revise the description

Signed-off-by: Luke Shumaker <[email protected]>
@LukeShu
py-list-deps: Add a hack to work with newer setuptools
076e18f 
This was causing problems for me on my laptop.

Signed-off-by: Luke Shumaker <[email protected]>
@LukeShu LukeShu force-pushed the lukeshu/clobber branch from fc9e910 to 0133a62 Compare May 19, 2022 09:34
LukeShu added 3 commits May 19, 2022 16:42
@LukeShu
build-sys: Don't say .PHONY: %.clean every dang time
b7ad4b2 
Signed-off-by: Luke Shumaker <[email protected]>
@LukeShu
envoy.mk: Fix a typoed line affecting make clobber
91d8150 
Signed-off-by: Luke Shumaker <[email protected]>
@LukeShu
build-sys: Add a default %.clean / %.rm implementation
6ce9a8d 
Avoid writing repetitive rules for simple files.

Signed-off-by: Luke Shumaker <[email protected]>
@LukeShu LukeShu force-pushed the lukeshu/clobber branch 4 times, most recently from b37c5c4 to ebe8068 Compare May 19, 2022 15:17
kflynn
kflynn reviewed May 19, 2022
View reviewed changes
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.

Looking like a very nice cleanup so far! Shall we add

maintainer-clean: clobber

in there somewhere?

build-aux/docs/intro.md Outdated Show resolved Hide resolved
.github/workflows/promote-ga.yml Outdated Show resolved Hide resolved
.github/actions/after-job/action.yml Outdated Show resolved Hide resolved
build-aux/generate.mk Show resolved Hide resolved
build-aux/main.mk Outdated Show resolved Hide resolved
build-aux/main.mk Show resolved Hide resolved
@LukeShu LukeShu force-pushed the lukeshu/clobber branch from ebe8068 to ab502dc Compare May 19, 2022 21:50
@LukeShu LukeShu force-pushed the lukeshu/clobber branch 2 times, most recently from 70a6602 to 4a52ea9 Compare May 19, 2022 22:39
@LukeShu LukeShu force-pushed the lukeshu/clobber branch 3 times, most recently from 2d18a67 to 670dee0 Compare May 20, 2022 09:40
LukeShu added 2 commits May 20, 2022 11:40
@LukeShu
.github: Have CI check that make clobber works for everything we do…
52cbe8b 
… in CI [ci-skip]

This is marked [ci-skip] because (as this PR reveals) `make clobber`
is currently broken.

Signed-off-by: Luke Shumaker <[email protected]>
@LukeShu
build-sys: Fix make clobber
670dee0 
Get it to actually clean up all the things that it should.

Signed-off-by: Luke Shumaker <[email protected]>
@LukeShu LukeShu marked this pull request as ready for review May 20, 2022 10:21
kflynn
kflynn approved these changes May 20, 2022
View reviewed changes
Copy link
Member

@kflynn kflynn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hit it! 🙂 Looking forward to this making life easier.

@LukeShu LukeShu merged commit f4d2459 into release/v2.3 May 20, 2022
@LukeShu LukeShu deleted the lukeshu/clobber branch May 20, 2022 11:33
@rick-a-lane-ii rick-a-lane-ii mentioned this pull request Apr 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Alice-Lilith Alice-Lilith Alice-Lilith approved these changes

@kflynn kflynn kflynn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LukeShu @kflynn @Alice-Lilith