Skip to content

Expanding tests and docs relating to deleting profiles#8351

Merged
Ericson2314 merged 4 commits intoNixOS:masterfrom
obsidiansystems:delete-profiles-tests-docs
Jun 14, 2023
Merged

Expanding tests and docs relating to deleting profiles#8351
Ericson2314 merged 4 commits intoNixOS:masterfrom
obsidiansystems:delete-profiles-tests-docs

Conversation

@Ericson2314
Copy link
Member

Motivation

Improve docs and tests of commands relating to deleting profiles.

Context

#8154 got me thinking about this stuff.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • documentation in the internal API docs
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label May 16, 2023
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

This is clearly an improvement and should prevent lots of confusion over how things work.

@Ericson2314
Copy link
Member Author

Thanks for the thorough review!

No comments on the test part though! Should we land that first? (I made it the first commit for a reason :D.)

@Ericson2314 Ericson2314 force-pushed the delete-profiles-tests-docs branch 4 times, most recently from 2f0dd64 to a31b3f9 Compare May 17, 2023 19:05
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-05-18-learning-journey-working-group-meeting-notes-9/28405/3

*Example*: `30d`

Delete all generations older than *days* days.
The generation that was active at that point in time is excluded, and will not be deleted.
Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't know what this is supposed to mean.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is taking the parenthetical from above and making it a standalone sentence --- I am not sure exactly either!

We can figure it out and make it clear in a follow-up PR. For now, it is at least no worse than before.

Copy link
Member Author

@Ericson2314 Ericson2314 Jun 14, 2023

Choose a reason for hiding this comment

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

This command will delete all generations older than 30 days, except for the generation that was active 30 days ago (if it currently exists).

This is my interpretation, from the examples.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe to slightly clarify then...

Suggested change
The generation that was active at that point in time is excluded, and will not be deleted.
The generation that was active at that point in the past is excluded, and will not be deleted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I like it. I will go read the code now to double check too.

Ericson2314 and others added 4 commits June 14, 2023 19:01
Good for test parallelism, and separation of concerns (core GC vs
profiles deleting).
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
@Ericson2314 Ericson2314 force-pushed the delete-profiles-tests-docs branch from ba323ed to 5b7e285 Compare June 14, 2023 23:03
@Ericson2314 Ericson2314 enabled auto-merge June 14, 2023 23:05
@Ericson2314 Ericson2314 merged commit 946cd9e into NixOS:master Jun 14, 2023
@Ericson2314 Ericson2314 deleted the delete-profiles-tests-docs branch June 14, 2023 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

with-tests Issues related to testing. PRs with tests have some priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants