[releng] PR #7366: add script for pruning atomic ostree repos

[Dusty Mabe]

dustymabe opened a new pull-request against the project: `releng` that you are following:
``
add script for pruning atomic ostree repos
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Kellin]

kellin commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
Why provide the --test argument at all if it sets test to true every time?
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
good question. The plan is to have this start off as test only and called from a [cron
job](https://lists.fedoraproject.org/archives/list/infrastructure@lists.f....
I'll then update this script to pass `args.test` instead of `True`. My thoughts were
it's easier to update this script than it is to update the cron job in infra git and
beg someone to re-run the playbook for me. 
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Kellin]

kellin commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
Where is this exception handled?

As far as I know, exceptions in scripts need handling unlike those in APIs where the
person calling the API is expected to handle exceptions appropriately for their program.
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
It isn't handled. The desire is that the script stop executing and throw an error. 
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Kellin]

kellin commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
Rather than comment this out here, I would have it call appropriately and do a logger.info
that states this feature is disabled due to ISSUE X, ISSUE Y.

Otherwise, I'd just remove all the code for this until the other bugs are ironed out.
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Kellin]

kellin commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
Throwing an error to the invoking shell can be handled with an error handler and the
appropriate sys.exit(EXIT_CODE) call.
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
right, i'm just not sure why I need to do that here vs just raising an exception, the
same goal is achieved either way?? 
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
added a log statement in the function and uncommented this so that it will call the
function
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
@kellin rebased
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Colin Walters]

walters commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
This seems fine for now but I'm curious if you considered using the ostree pygobject
bindings?  There's various examples in
https://github.com/ostreedev/ostree-releng-scripts

Related to this, I wonder if it'd make sense to "parse" the refs (basically
`prefix/${arch}/${suffix}`) rather than having a separate list?

But in the end rojig will give us this arch-independent pruning naturally.
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
>  This seems fine for now but I'm curious if you considered using
> the ostree pygobject bindings? There's various examples in
> https://github.com/ostreedev/ostree-releng-scripts
>  
I did consider it, but am much more familiar with the command line so stuck to that for
now.

>  Related to this, I wonder if it'd make sense to "parse"
> the refs (basically prefix/${arch}/${suffix}) rather than having a separate list?
>  
so this would make `PROD_REFS` shorter right? I thought about that as well, just
didn't take the time to figure it out. 

>  But in the end rojig will give us this arch-independent pruning
> naturally.
>  
yeah, we'll have to plan and make that a priority soon. 
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Sinny Kumari]

sinnykumari commented on the pull-request: `add script for pruning atomic ostree repos`
that you are following:
``
Shouldn't ref here be 'fedora/28/x86_64/testing/atomic-host instead' of
'fedora/28/x86_64/updates/atomic-host' ? Similarly, for other arches.
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Sinny Kumari]

sinnykumari commented on the pull-request: `add script for pruning atomic ostree repos`
that you are following:
``
I think you meant to say testing instead of updates.
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
indeed. fixed now. thanks
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
yep. fixed now. 
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
@sinnykumari - comments addressed
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Sinny Kumari]

sinnykumari commented on the pull-request: `add script for pruning atomic ostree repos`
that you are following:
``
:+1:
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
rebased on latest master.. I'd like +1 from @kellin @mohanboddu and @puiterwijk if
possible please. 
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Mohan Boddu]

mohanboddu commented on the pull-request: `add script for pruning atomic ostree repos`
that you are following:
``
Better use some other variable name for PROD_REFS which conflicts with prod_refs, it works
but better use something else to eliminate confusion
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Kellin]

kellin commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
I would prefer if this set default=True here and then passed that to all the other
functions that way. This way when we shift it in production we simply set it to
--test=False on the command line rather than have to update the function.  It also ensures
that, for anyone, unless they explicitly ever tell it to prune it will always do a dry run
instead.

I feel that would be safer behavior for the long term.
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
will move `prod_refs` to `prodrefs`. 
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Kellin]

kellin commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
Adding on here to what @mohanboddu said - I think if this were named PROD_REF_POLICIES it
would be clearer that it's a dictionary and have less confusion with prod_refs.
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Kellin]

kellin commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
I'm picking nits, but I think if we need to exit here when the policy is none, this
should be a positive statement.

EG:

if policy is None:
    logger....
    continue

it eliminates the extra else line and has more clarity.
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Kellin]

kellin commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
This is a question about the longer term and not necessary something that needs solved to
deploy this script today.

Do we care about what gets pruned over time and is it kept anywhere else? I almost feel
this should be a consumable format like JSON that lets us see which refs got pruned and
when if we care.

I'm not a fan of logging to text and then having it sit somewhere unconsumed.
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
+1
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
+1
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
I think it's mostly log statements for now. If we moved to using the python library
then we might be able to get more info out. 
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
I changed it a bit. Once I do a new upload see if I've addressed this comment
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Kellin]

kellin commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
This comment is assuming the script will be changed to always run as test unless
--test=False to run in production.

I would change the test to true here at the start of the method call so that even if we
run it in production, it's still a test.

I think that gives us the effect you're going for of having the stub but never
allowing it to actually run until it's fixed.
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
I think I've addressed this in main(). please see my next upload when I do it and let
me know what you think
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
also see the comments and the pre-mature `return` directly below this code
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Kellin]

kellin commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
I went and re-read the argparse manual and realized you can't change the default of
this from the command line.

So what I really want, and I apologize for being unclear due to that misunderstanding on
my part, is for the script to *ONLY* run for real if something is passed.

Can we change this to --dry-run and that passes the test case instead?

This negates the need to set a variable later in the script; we just change the job to
remove the passing of --dry-run.
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Kellin]

kellin commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
I would also say then that "test" should be renamed to "dry_run".
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Kellin]

kellin commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
This could have stayed prod_ref - PEP standard for names. I know it's a nit, but, the
other change made the difference clear already for us.
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
In my mind there is really no difference between passing `--test` and passing `--dry-run`.
It sounds like what you want is a `--notest` option. I guess that would be ok but seems a
bit odd. 
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
ok
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Kellin]

kellin commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
What I don't like is the manual setting of args.test = True in the body.

Ideally, what I'd *prefer* here is that the script when invoked always goes dry-run
and we have some option that actually performs the action.

I just can't remember the right word for the standard on this...

EG: --dry-run for scripts whose behavior defaults to run in production, and something else
for not.

The idea of having something we have to delete from code bothers me; we should make sure
we set it --dry-run in the cron job or, since this is a destructive script, opt to make
the default behavior not do anything but show the changes and then a single option
--really-do-it (but that's a terrible name) that does the changes for real.
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Mohan Boddu]

mohanboddu commented on the pull-request: `add script for pruning atomic ostree repos`
that you are following:
``
@dustymabe Please sign the commits or else we cannot merge them.
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
@mohanboddu - I'm going to squash all commits before I merge. I also want patrick to
review before I merge as well. 
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Suzanne Yeghiayan]

syeghiay commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
@dustymabe , were you able to get @puiterwijk to review this yet?
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
not yet, sorry
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Suzanne Yeghiayan]

The pull-request: `add script for pruning atomic ostree repos` of project: `releng` has
been assigned to `mohanboddu` by syeghiay.

https://pagure.io/releng/pull-request/7366

[Kevin Fenzi]

kevin commented on the pull-request: `add script for pruning atomic ostree repos` that you
are following:
``
This needs rebasing again... @dustymabe can you do that when you get back?

``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
yep - unless @sinnykumari wants to pick it up before I get back
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Sinny Kumari]

sinnykumari commented on the pull-request: `add script for pruning atomic ostree repos`
that you are following:
``
>  yep - unless @sinnykumari wants to pick it up before I get back
>  
sure, I will see if I can pick this up before you are back

Meanwhile, is there any related issue where ostree repo pruning has been discussed? I
don't recall much,  will be useful to have some context.
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366

[Dusty Mabe]

dustymabe commented on the pull-request: `add script for pruning atomic ostree repos` that
you are following:
``
i cant rememer any other than this ticket. it's mostly something i started to look at
when we started making statuc deltas and realizing the repo was going to get large fast if
we don't prune. 
``

To reply, visit the link below or just reply to this email
https://pagure.io/releng/pull-request/7366