Fix the clean ami script to remove the amis based on date

sayanchowdhury commented

2019-02-20 16:42:26 +00:00

Contributor

This fixes #8126.

Additional changes made to the script:

You can pass a additional params as days. Earlier it was hardcoded to 10 days. This is beneficial if the script fails to run over 10 days then we can pass the --days param.
Removes images which have date in them (treats them as nightly) except for the updates composes because I'm unable to find a way to filter out the 2WA released composes.

Signed-off-by: Sayan Chowdhury sayan.chowdhury2012@gmail.com

This fixes #8126. Additional changes made to the script: - You can pass a additional params as days. Earlier it was hardcoded to 10 days. This is beneficial if the script fails to run over 10 days then we can pass the `--days` param. - Removes images which have date in them (treats them as nightly) except for the updates composes because I'm unable to find a way to filter out the 2WA released composes. Signed-off-by: Sayan Chowdhury <sayan.chowdhury2012@gmail.com>

dustymabe commented

2019-02-21 14:28:58 +00:00

Member

days=10 is hardcoded here - is that right? I think it should be days=days

days=10 is hardcoded here - is that right? I think it should be `days=days`

dustymabe commented

2019-02-21 14:31:45 +00:00

Member

can we re-order these two (change perms first and then delete) and then add a nice comment about what is going on?

dustymabe commented

2019-02-21 14:36:16 +00:00

Member

It took me a while to figure out what is going on and I'm still not sure it's entirely right (or maybe I misunderstand).

For each AMI:

(2*$days < item < $days) -> DELETE
($days < item < TODAY) -> CHANGE PERMISSIONS

Is that right?

It took me a while to figure out what is going on and I'm still not sure it's entirely right (or maybe I misunderstand). For each AMI: - `(2*$days < item < $days)` -> **DELETE** - `($days < item < TODAY)` -> **CHANGE PERMISSIONS** Is that right?

sayanchowdhury commented

2019-02-21 14:37:12 +00:00

Author

Contributor

Here, 10 is the number of days the AMIs will stay with the changed perms.

dustymabe commented

2019-02-21 14:37:16 +00:00

Member

This help message needs more clarity. I don't quite get exactly what value you want me to give it for days.

dustymabe commented

2019-02-21 14:38:04 +00:00

Member

we need a comment at the beginning of this if/else that explains how we are determining which AMIs are OK to delete.

dustymabe commented

2019-02-21 14:38:55 +00:00

Member

actually maybe better to just explain all of the logic before any of the if statements in this function.

dustymabe commented

2019-02-21 14:39:38 +00:00

Member

REGIONS = (          
    'us-east-1',     
    'us-west-2',     
    'us-west-1',     
    'eu-west-1',     
    'eu-central-1',  
    'ap-south-1',    
    'ap-southeast-1',
    'ap-northeast-1',
    'ap-northeast-2',
    'ap-southeast-2',
    'sa-east-1',     
)

Do we have more regions now?

``` REGIONS = ( 'us-east-1', 'us-west-2', 'us-west-1', 'eu-west-1', 'eu-central-1', 'ap-south-1', 'ap-southeast-1', 'ap-northeast-1', 'ap-northeast-2', 'ap-southeast-2', 'sa-east-1', ) ``` Do we have more regions now?

sayanchowdhury commented

2019-02-21 14:43:26 +00:00

Author

Contributor

rebased onto e8bb003aa24db1ec202545ad378be1553e7eb7a4

sinnykumari commented

2019-02-21 14:51:19 +00:00

Contributor

We have nightly AMIs in two additional regions:
ca-central-1
eu-west-2

We have nightly AMIs in two additional regions: ca-central-1 eu-west-2

sayanchowdhury commented

2019-02-21 15:26:41 +00:00

Author

Contributor

rebased onto 41b8936aaf72746038704b7b51542ef5367044b8

sayanchowdhury commented

2019-02-21 15:26:50 +00:00

Author

Contributor

can we re-order these two (change perms first and then delete) and then add a nice comment about what is going on?

Done.

> can we re-order these two (change perms first and then delete) and then add a nice comment about what is going on? Done.

sinnykumari commented

2019-02-21 15:30:38 +00:00

Contributor

Looks good to me, one minor change may required:
we might want to make NIGHTLY lowercase at pagure.io/fork/sayanchowdhury/releng@0ce51d0036/f/scripts/clean-amis.py (_95) , since result of fedfind is somewhat like this for rawhide and F30 nightly:

>>> compose_info = fedfind.release.get_release(cid='Fedora-Rawhide-20190213.n.0')
>>> print compose_info.type
nightly

Also, this won't delete any of Fedora updates and updates testing AMIs, since for both cases compose_info.label starts with Update-*

Looks good to me, one minor change may required: we might want to make NIGHTLY lowercase at https://pagure.io/fork/sayanchowdhury/releng/blob/0ce51d0036c4b779e71efdc4fdfb0e8bae369107/f/scripts/clean-amis.py#_95 , since result of fedfind is somewhat like this for rawhide and F30 nightly: ``` >>> compose_info = fedfind.release.get_release(cid='Fedora-Rawhide-20190213.n.0') >>> print compose_info.type nightly ``` Also, this won't delete any of Fedora updates and updates testing AMIs, since for both cases compose_info.label starts with Update-*

sayanchowdhury commented

2019-02-21 15:32:30 +00:00

Author

Contributor

(2*$days < item < $days) -> DELETE
($days < item < TODAY) -> CHANGE PERMISSIONS

for delete, its $days-10 < item < TODAY-10 -> DELETE

> (2*$days < item < $days) -> DELETE > ($days < item < TODAY) -> CHANGE PERMISSIONS for delete, its $days-10 < item < TODAY-10 -> DELETE

sayanchowdhury commented

2019-02-21 15:35:30 +00:00

Author

Contributor

Looks good to me, one minor change may required:
we might want to make NIGHTLY lowercase at pagure.io/fork/sayanchowdhury/releng@0ce51d0036/f/scripts/clean-amis.py (_95) , since result of fedfind is somewhat like this for rawhide and F30 nightly:

compose_info = fedfind.release.get_release(cid='Fedora-Rawhide-20190213.n.0')
print compose_info.type
nightly

Yeah, NIGHTLY is the variable. The value of it is nightly.

Also, this won't delete any of Fedora updates and updates testing AMIs, since for both cases compose_info.label starts with Update-*

Yes, This is to prevent from deleting the 2WA Host released AMIs. I could not find a way to distinguish from the other AMIs.

> Looks good to me, one minor change may required: > we might want to make NIGHTLY lowercase at https://pagure.io/fork/sayanchowdhury/releng/blob/0ce51d0036c4b779e71efdc4fdfb0e8bae369107/f/scripts/clean-amis.py#_95 , since result of fedfind is somewhat like this for rawhide and F30 nightly: > >>> compose_info = fedfind.release.get_release(cid='Fedora-Rawhide-20190213.n.0') > >>> print compose_info.type > nightly > Yeah, `NIGHTLY` is the variable. The value of it is `nightly`. > > Also, this won't delete any of Fedora updates and updates testing AMIs, since for both cases compose_info.label starts with Update-* Yes, This is to prevent from deleting the 2WA Host released AMIs. I could not find a way to distinguish from the other AMIs.

sinnykumari commented

2019-02-21 16:47:51 +00:00

Contributor

Yeah, NIGHTLY is the variable. The value of it is nightly.

ah yes, missed it.

Also, this won't delete any of Fedora updates and updates testing AMIs, since for both cases compose_info.label starts with Update-*

Yes, This is to prevent from deleting the 2WA Host released AMIs. I could not find a way to distinguish from the other AMIs.

Compose ID of updates are usually Fedora-$releases-updates-$composedate and for updates-testing, it is Fedora-$releases-updates-testing-$composedate .
We do 2WA release from updates compose, so maybe we can filter out at compose ID level to remove AMIs from updates-testing?
@dustymabe WDYT

> Yeah, NIGHTLY is the variable. The value of it is nightly. ah yes, missed it. > Also, this won't delete any of Fedora updates and updates testing AMIs, since for both cases compose_info.label starts with Update-* > > Yes, This is to prevent from deleting the 2WA Host released AMIs. I could not find a way to distinguish from the other AMIs. Compose ID of updates are usually Fedora-$releases-updates-$composedate and for updates-testing, it is Fedora-$releases-updates-testing-$composedate . We do 2WA release from updates compose, so maybe we can filter out at compose ID level to remove AMIs from updates-testing? @dustymabe WDYT

sinnykumari commented

2019-02-21 17:02:55 +00:00

Contributor

@sayanchowdhury One question for my clarity, does using topic org.fedoraproject.prod.fedimg.image.upload gets us to fetch AMIs for all desired regions we want AMIs to make private or delete or we should move to org.fedoraproject.prod.fedimg.image.publish topic where we have AMI IDs for all regions available?

@sayanchowdhury One question for my clarity, does using topic [org.fedoraproject.prod.fedimg.image.upload]( https://pagure.io/fork/sayanchowdhury/releng/blob/a7487345d6a3aa1af50acb6dad116fddd93a55c2/f/scripts/clean-amis.py#_93) gets us to fetch AMIs for all desired regions we want AMIs to make private or delete or we should move to `org.fedoraproject.prod.fedimg.image.publish` topic where we have AMI IDs for all regions available?

sayanchowdhury commented

2019-02-21 17:10:13 +00:00

Author

Contributor

Yeah, NIGHTLY is the variable. The value of it is nightly.

ah yes, missed it.

Also, this won't delete any of Fedora updates and updates testing AMIs, since for both cases compose_info.label starts with Update-*
Yes, This is to prevent from deleting the 2WA Host released AMIs. I could not find a way to distinguish from the other AMIs.

Compose ID of updates are usually Fedora-$releases-updates-$composedate and for updates-testing, it is Fedora-$releases-updates-testing-$composedate .
We do 2WA release from updates compose, so maybe we can filter out at compose ID level to remove AMIs from updates-testing?
@dustymabe WDYT

We can do this. But still updates composes would stay.

> > Yeah, NIGHTLY is the variable. The value of it is nightly. > > ah yes, missed it. > > Also, this won't delete any of Fedora updates and updates testing AMIs, since for both cases compose_info.label starts with Update-* > Yes, This is to prevent from deleting the 2WA Host released AMIs. I could not find a way to distinguish from the other AMIs. > > Compose ID of updates are usually Fedora-$releases-updates-$composedate and for updates-testing, it is Fedora-$releases-updates-testing-$composedate . > We do 2WA release from updates compose, so maybe we can filter out at compose ID level to remove AMIs from updates-testing? > @dustymabe WDYT We can do this. But still updates composes would stay.

dustymabe commented

2019-02-21 17:32:08 +00:00

Member

Let's define a new variable at the top of the script (WAIT_PERIOD=10)

Let's define a new variable at the top of the script (`WAIT_PERIOD=10`)

dustymabe commented

2019-02-21 17:33:35 +00:00

Member

i'm not sure if we should use days here at all. we basically want any AMIs that have had launch permissions removed for more than 10 days right? I almost wonder if we should be using fedmsg at all and not pure ec2 commands for this.

i'm not sure if we should use `days` here at all. we basically want any AMIs that have had launch permissions removed for more than 10 days right? I almost wonder if we should be using fedmsg at all and not pure ec2 commands for this.

dustymabe commented

2019-02-21 17:36:59 +00:00

Member

we should be able to remove updates-testing AMIs IMHO. Can we use any other identifying information?

looks like @sinnykumari already said this above :) - sorry @sinnykumari

we should be able to remove updates-testing AMIs IMHO. Can we use any other identifying information? looks like @sinnykumari already said this above :) - sorry @sinnykumari - [updates compose metadata](https://kojipkgs.fedoraproject.org/compose/updates/Fedora-29-updates-20190221.0/compose/metadata/composeinfo.json) - [updates-testing compose metadata](https://kojipkgs.fedoraproject.org/compose/updates/Fedora-29-updates-testing-20190221.0/compose/metadata/composeinfo.json)

dustymabe commented

2019-02-21 17:38:37 +00:00

Member

If I upload an AMI today and this gets run today does that AMI get its launch permissions removed?

sayanchowdhury commented

2019-02-22 14:09:51 +00:00

Author

Contributor

rebased onto 6a46caca939ed6620d2ed022b5b4bcbb097093a7

sayanchowdhury commented

2019-02-25 16:14:22 +00:00

Author

Contributor

rebased onto 731affa16d9b413f1539a475d23b9180a2df87bc

sayanchowdhury commented

2019-02-25 19:01:06 +00:00

Author

Contributor

rebased onto d41e4dabfbd4734f828decae3e3dadffbdaed7d7

sayanchowdhury commented

2019-02-25 19:08:13 +00:00

Author

Contributor

@dustymabe I fixed the issue that you pointed out. Now AMIs $((today-5)-$days) < item < $(today-5) will be deleted.

sayanchowdhury commented

2019-02-25 19:09:40 +00:00

Author

Contributor

Since the AMIs will be deleted after 10 days, so the $(today-15)-days < item < today -15

dustymabe commented

2019-02-25 20:40:15 +00:00

Member

The benefit of using this doesn't seem to be worth it to me. We define more twice and we call it twice and all but one argument is static. Let's just call get_page() instead rather than making readers lookup what functools.partial does?

The benefit of using this doesn't seem to be worth it to me. We define more twice and we call it twice and all but one argument is static. Let's just call get_page() instead rather than making readers lookup what `functools.partial` does?

dustymabe commented

2019-02-25 20:42:24 +00:00

Member

add comment above this.

dustymabe commented

2019-02-25 20:43:38 +00:00

Member

why should we include AMIs that have the date in them? What does it mean if they do have the date in them?

dustymabe commented

2019-02-25 20:43:54 +00:00

Member

What does this mean? Add comment

dustymabe commented

2019-02-25 20:48:12 +00:00

Member

these should be useful for the dry-run case, no?

dustymabe commented

2019-02-25 21:09:21 +00:00

Member

Here are some slight modifications that would make this more useful I think:

diff --git a/scripts/clean-amis.py b/scripts/clean-amis.py
index 31d069c..b518089 100755
--- a/scripts/clean-amis.py
+++ b/scripts/clean-amis.py
@@ -265,8 +265,16 @@ if __name__ == '__main__':
         help="Delete the AMIs whose launch permissions have been removed",
         action="store_true", default=False)
     argument_parser.add_argument(
-        "--days",
-        help="Specify the number of days to run the operation",
+        "--howfarback",
+        help="How far back should we search for AMIs that need to be deleted",
+        type=int)
+    argument_parser.add_argument(
+        "--deletewaitperiod",
+        help="How long to wait after launch perms were removed before deleting",
+        type=int)
+    argument_parser.add_argument(
+        "--permswaitperiod",
+        help="Specify the number of days to wait before removing permissions",
         type=int)
     argument_parser.add_argument(
         "--change-perms",
@@ -284,9 +292,22 @@ if __name__ == '__main__':
     if args.delete and args.change_perms:
         print('Both the argument delete and change permission is not allowed')
 
-    days = 10
-    if args.days:
-        days = args.days
+    # Default to waiting 10 days after launch permissions have been
+    # removed before deleting
+    deletewaitperiod = 10
+    if args.deletewaitperiod:
+        deletewaitperiod
+
+    permswaitperiod = 10
+    if args.permswaitperiod:
+        permswaitperiod = args.permswaitperiod
+
+    # Ideally we could search through all the AMIs that ever were
+    # created but this would create quite the load on datagrepper.
+    # default to 4 weeks
+    howfarback = 28 # 4 weeks
+    if args.howfarback:
+        howfarback = args.howfarback
 
     # The AMIs deleted are the nightly AMIs that are uploaded via fedimg everyday.
     # The clean up of the AMIs happens through a cron job.
@@ -296,12 +317,20 @@ if __name__ == '__main__':
     # - If no issues are reported in 10 days, the AMIs are deleted permanently.
 
     if args.change_perms:
-        end = (datetime.now() - timedelta(days=5)).strftime('%s')
-        amis = _get_nightly_amis_nd(delta=86400 * days, end=int(end))
+        if howfarback < permswaitperiod:
+            raise Exception # shouldn't happen
+        end = (datetime.now() - timedelta(days=permswaitperiod)).strftime('%s')
+        amis = _get_nightly_amis_nd(delta=86400 * howfarback, end=int(end))
         change_amis_permission_nd(amis, dry_run=args.dry_run)
 
     if args.delete:
-        end = (datetime.now() - timedelta(days=15)).strftime('%s')
-        amis = _get_nightly_amis_nd(delta=86400 * days, end=int(end))
+        if howfarback < deletewaitperiod:
+            raise Exception # shouldn't happen
+        end = (datetime.now() - timedelta(days=deletewaitperiod)).strftime('%s')
+        amis = _get_nightly_amis_nd(delta=86400 * howfarback, end=int(end))
+        # Here we need to see how long it's been since we removed the
+        # permissions. If it has been deletewaitperiod days then we can
+        # delete. If there is no way to tell this organically then we
+        # could add a tag to the AMI with a timestamp at the same time
+        # we remove the permissions.
         delete_amis_nd(amis, dry_run=args.dry_run)
-

note the comment about possibly using tags set on the AMIs to determine when
the launch permissions were removed.

Here are some slight modifications that would make this more useful I think: ```patch diff --git a/scripts/clean-amis.py b/scripts/clean-amis.py index 31d069c..b518089 100755 --- a/scripts/clean-amis.py +++ b/scripts/clean-amis.py @@ -265,8 +265,16 @@ if __name__ == '__main__': help="Delete the AMIs whose launch permissions have been removed", action="store_true", default=False) argument_parser.add_argument( - "--days", - help="Specify the number of days to run the operation", + "--howfarback", + help="How far back should we search for AMIs that need to be deleted", + type=int) + argument_parser.add_argument( + "--deletewaitperiod", + help="How long to wait after launch perms were removed before deleting", + type=int) + argument_parser.add_argument( + "--permswaitperiod", + help="Specify the number of days to wait before removing permissions", type=int) argument_parser.add_argument( "--change-perms", @@ -284,9 +292,22 @@ if __name__ == '__main__': if args.delete and args.change_perms: print('Both the argument delete and change permission is not allowed') - days = 10 - if args.days: - days = args.days + # Default to waiting 10 days after launch permissions have been + # removed before deleting + deletewaitperiod = 10 + if args.deletewaitperiod: + deletewaitperiod + + permswaitperiod = 10 + if args.permswaitperiod: + permswaitperiod = args.permswaitperiod + + # Ideally we could search through all the AMIs that ever were + # created but this would create quite the load on datagrepper. + # default to 4 weeks + howfarback = 28 # 4 weeks + if args.howfarback: + howfarback = args.howfarback # The AMIs deleted are the nightly AMIs that are uploaded via fedimg everyday. # The clean up of the AMIs happens through a cron job. @@ -296,12 +317,20 @@ if __name__ == '__main__': # - If no issues are reported in 10 days, the AMIs are deleted permanently. if args.change_perms: - end = (datetime.now() - timedelta(days=5)).strftime('%s') - amis = _get_nightly_amis_nd(delta=86400 * days, end=int(end)) + if howfarback < permswaitperiod: + raise Exception # shouldn't happen + end = (datetime.now() - timedelta(days=permswaitperiod)).strftime('%s') + amis = _get_nightly_amis_nd(delta=86400 * howfarback, end=int(end)) change_amis_permission_nd(amis, dry_run=args.dry_run) if args.delete: - end = (datetime.now() - timedelta(days=15)).strftime('%s') - amis = _get_nightly_amis_nd(delta=86400 * days, end=int(end)) + if howfarback < deletewaitperiod: + raise Exception # shouldn't happen + end = (datetime.now() - timedelta(days=deletewaitperiod)).strftime('%s') + amis = _get_nightly_amis_nd(delta=86400 * howfarback, end=int(end)) + # Here we need to see how long it's been since we removed the + # permissions. If it has been deletewaitperiod days then we can + # delete. If there is no way to tell this organically then we + # could add a tag to the AMI with a timestamp at the same time + # we remove the permissions. delete_amis_nd(amis, dry_run=args.dry_run) - ``` note the comment about possibly using tags set on the AMIs to determine when the launch permissions were removed.

dustymabe commented

2019-02-27 19:39:25 +00:00

Member

any updates for today for me to review?

sayanchowdhury commented

2019-02-27 20:35:16 +00:00

Author

Contributor

1 new commit added

Tmp commit

**1 new commit added** * ``Tmp commit``

sayanchowdhury commented

2019-02-27 20:51:12 +00:00

Author

Contributor

rebased onto 95aca465273674f9b47c12160c620a317fe02d72

sayanchowdhury commented

2019-02-27 20:53:12 +00:00

Author

Contributor

rebased onto 28179f2189a3dd32b5b2cfefa9a12d476f413139

sayanchowdhury commented

2019-02-27 20:54:28 +00:00

Author

Contributor

rebased onto 2d0a536a680e1899acccad7a2e22bd8ac22d4efc

sayanchowdhury commented

2019-02-27 20:58:41 +00:00

Author

Contributor

rebased onto 68aafd7315ccbfa5f51215b1a5a6db26b6bafa06

sayanchowdhury commented

2019-02-27 21:01:32 +00:00

Author

Contributor

rebased onto 9beff8aacd778409a724b1845be4cb24a815668f

dustymabe commented

2019-02-27 21:06:00 +00:00

Member

Specify the number of days worth of AMI fedmsg information to fetch from datagrepper.

dustymabe commented

2019-02-27 21:06:28 +00:00

Member

probably need to raise an exception here?

dustymabe commented

2019-02-27 21:07:40 +00:00

Member

I don't understand the 2nd if statement here and below for deletewaitperiod.

dustymabe commented

2019-02-27 21:08:23 +00:00

Member

need description error message here

dustymabe commented

2019-02-27 21:11:11 +00:00

Member

Need to fix function description and arg descrition here

dustymabe commented

2019-02-27 21:14:16 +00:00

Member

This is a tag key/value pair we are defining here right? I would do something like Key=LaunchPermissionRevoked and Value=todaystimestamp.

This is a tag key/value pair we are defining here right? I would do something like `Key=LaunchPermissionRevoked` and `Value=todaystimestamp`.

dustymabe commented

2019-02-27 21:19:13 +00:00

Member

this seems limiting - we will only delete images that were deleted on that day. We want to delete all the ones with permissions revoked before that day. I'm thinking we should get all images with this tag (key) and then filter the list of AMIs based on the timestamp (value).

this seems limiting - we will only delete images that were deleted on that day. We want to delete all the ones with permissions revoked *before* that day. I'm thinking we should get all images with this tag (key) and then filter the list of AMIs based on the timestamp (value).

sayanchowdhury commented

2019-02-27 21:24:44 +00:00

Author

Contributor

"on or before that day" or "before that day"

sayanchowdhury commented

2019-02-27 21:25:27 +00:00

Author

Contributor

This is when you want to delete same day images.

dustymabe commented

2019-02-27 21:29:21 +00:00

Member

I don't know if I support deleting same day images but I guess it makes sense for development. The user should be able to pass --permswaitperiod 0, right? We should not need a special if statement for this.

I don't know if I support deleting same day images but I guess it makes sense for development. The user should be able to pass `--permswaitperiod 0`, right? We should not need a special if statement for this.

dustymabe commented

2019-02-27 21:30:20 +00:00

Member

"on or before that day" works for me if it does for you. but in general "off by one" shouldn't matter too much here.

sayanchowdhury commented

2019-02-27 21:32:43 +00:00

Author

Contributor

I don't know if I support deleting same day images but I guess it makes sense for development. The user should be able to pass --permswaitperiod 0, right? We should not need a special if statement for this.

since it's 0, the if condition evaluated to false and the default value of 10 is set. And yes it's needed for development purpose.

> I don't know if I support deleting same day images but I guess it makes sense for development. The user should be able to pass --permswaitperiod 0, right? We should not need a special if statement for this. since it's 0, the if condition evaluated to false and the default value of 10 is set. And yes it's needed for development purpose.

sayanchowdhury commented

2019-02-27 21:33:07 +00:00

Author

Contributor

"on or before that day" works for me if it does for you. but in general "off by one" shouldn't matter too much here.

+1

> "on or before that day" works for me if it does for you. but in general "off by one" shouldn't matter too much here. +1

dustymabe commented

2019-02-27 21:35:57 +00:00

Member

I don't know if I support deleting same day images but I guess it makes sense for development. The user should be able to pass --permswaitperiod 0, right? We should not need a special if statement for this.

since it's 0, the if condition evaluated to false and the default value of 10 is set. And yes it's needed for development purpose.

I see. In that case we should change things up a bit. Let's set the defaults for these values in argparse calls above and then just use args.days args.permswaitperiod and args.deletewaitperiod everywhere - WDYT?

> > I don't know if I support deleting same day images but I guess it makes sense for development. The user should be able to pass --permswaitperiod 0, right? We should not need a special if statement for this. > > since it's 0, the if condition evaluated to false and the default value of 10 is set. And yes it's needed for development purpose. I see. In that case we should change things up a bit. Let's set the defaults for these values in argparse calls above and then just use `args.days` `args.permswaitperiod` and `args.deletewaitperiod` everywhere - WDYT?

sayanchowdhury commented

2019-02-27 21:40:05 +00:00

Author

Contributor

I see. In that case we should change things up a bit. Let's set the defaults for these values in argparse calls above and then just use args.days args.permswaitperiod and args.deletewaitperiod everywhere - WDYT?

Sounds good to me

> I see. In that case we should change things up a bit. Let's set the defaults for these values in argparse calls above and then just use args.days args.permswaitperiod and args.deletewaitperiod everywhere - WDYT? Sounds good to me

sayanchowdhury commented

2019-02-27 22:37:31 +00:00

Author

Contributor

1 new commit added

Temp commit before squash

**1 new commit added** * ``Temp commit before squash``

dustymabe commented

2019-02-27 22:51:51 +00:00

Member

just checking - not sure what ami['Tags'] returns but i would think this should be tag.keys() and not tag.values.. Do you have an example of what tags=ami['Tags'] returns?

just checking - not sure what `ami['Tags']` returns but i would think this should be `tag.keys()` and not tag.values.. Do you have an example of what `tags=ami['Tags']` returns?

dustymabe commented

2019-02-27 22:53:18 +00:00

Member

i would make the function args here generic.. ie timestamp1, timestamp2 and add a description: eturns true if timestamp1 is greater than or equal to timestamp2

i would make the function args here generic.. ie timestamp1, timestamp2 and add a description: `eturns true if timestamp1 is greater than or equal to timestamp2`

dustymabe commented

2019-02-27 22:54:22 +00:00

Member

maybe rename the function to _timestamp_greater() the readability of the later code benefits from this

maybe rename the function to `_timestamp_greater()` the readability of the later code benefits from this

dustymabe commented

2019-02-27 22:55:28 +00:00

Member

this shouldn't happen. I would at least print out a warning message with information

sayanchowdhury commented

2019-02-27 22:56:12 +00:00

Author

Contributor

just checking - not sure what ami['Tags'] returns but i would think this should be tag.keys() and not tag.values.. Do you have an example of what tags=ami['Tags'] returns?

[{'Key': 'LaunchPermissionRevoked', 'Value': '27022019'}]

List of dictionaries

> just checking - not sure what ami['Tags'] returns but i would think this should be tag.keys() and not tag.values.. Do you have an example of what tags=ami['Tags'] returns? [{'Key': 'LaunchPermissionRevoked', 'Value': '27022019'}] List of dictionaries

sayanchowdhury commented

2019-02-27 22:58:23 +00:00

Author

Contributor

1 new commit added

Another temp commit

**1 new commit added** * ``Another temp commit``

dustymabe commented

2019-02-27 22:59:12 +00:00

Member

add a comment before this if statement - something like

# The revoke timestamp allows us to tell how long ago an image had permissions removed
# If the permissions have been removed for shorter than the waiting period then we can't
# delete it yet.

maybe we should change the function to be titled is_timestamp_older()

add a comment before this if statement - something like ``` # The revoke timestamp allows us to tell how long ago an image had permissions removed # If the permissions have been removed for shorter than the waiting period then we can't # delete it yet. ``` maybe we should change the function to be titled `is_timestamp_older()`

sayanchowdhury commented

2019-02-27 23:06:53 +00:00

Author

Contributor

3 new commits added

Another temp commit
Temp commit before squash
Fix the clean ami script to remove the amis based on date

**3 new commits added** * ``Another temp commit`` * ``Temp commit before squash`` * ``Fix the clean ami script to remove the amis based on date``

sayanchowdhury commented

2019-02-27 23:26:28 +00:00

Author

Contributor

1 new commit added

Temp commit

**1 new commit added** * ``Temp commit``

dustymabe commented

2019-02-27 23:29:41 +00:00

Member

i may have given you a bug - not sure if the string can span two lines here. maybe test it and see.

dustymabe commented

2019-02-27 23:30:09 +00:00

Member

LGTM - other than the one comment.

@sinnykumari - can you review this in the morning?

LGTM - other than the one comment. @sinnykumari - can you review this in the morning?

sayanchowdhury commented

2019-02-27 23:30:54 +00:00

Author

Contributor

1 new commit added

Format the script with black

**1 new commit added** * ``Format the script with black``

sayanchowdhury commented

2019-02-27 23:34:36 +00:00

Author

Contributor

rebased onto 9abfb34f4f2539febcb09ee4f608caae73f7e11f

sayanchowdhury commented

2019-02-27 23:35:38 +00:00

Author

Contributor

i may have given you a bug - not sure if the string can span two lines here. maybe test it and see.

Yeah I had fixed that in the next commit.

> i may have given you a bug - not sure if the string can span two lines here. maybe test it and see. Yeah I had fixed that in the next commit.

sayanchowdhury commented

2019-02-27 23:51:08 +00:00

Author

Contributor

rebased onto ba567ec22576d6304a0d6f028f2fbe7cb942ef43

sinnykumari commented

2019-02-28 06:48:47 +00:00

Contributor

Note that by this method we will be able to fetch TwoWeek compose id only for recent ones (from the time we added multi-arch support in twoweek.complete fedmsg topic). If we are interested to deleting most of the older composes, then we will have to add another condition to get composeID, sample json is https://apps.fedoraproject.org/datagrepper/id?id=2018-01994496-c703-49d6-8ffc-e99afd07c406&is_raw=true&size=extra-large

sinnykumari commented

2019-02-28 06:50:46 +00:00

Contributor

This idea came to my mind today as well, nice to see that this is already taken care of!

sinnykumari commented

2019-02-28 06:53:26 +00:00

Contributor

Added one concern above, other than that LGTM.

sayanchowdhury commented

2019-02-28 14:54:50 +00:00

Author

Contributor

rebased onto 1d716c707effae0a0542a397eec5e74ab1a5beb6

sayanchowdhury commented

2019-02-28 15:00:21 +00:00

Author

Contributor

2 new commits added

Format the script with black
Fix the clean ami script to remove the amis based on date

**2 new commits added** * ``Format the script with black`` * ``Fix the clean ami script to remove the amis based on date``

sinnykumari commented

2019-02-28 17:31:40 +00:00

Contributor

LGTM, have re-looked only into the modified content which I believe would be the "older-format fedmsg messages" section

dustymabe commented

2019-02-28 18:13:31 +00:00

Member

need to set default value here

dustymabe commented

2019-02-28 18:13:51 +00:00

Member

need to set default value here right?

dustymabe commented

2019-02-28 18:15:43 +00:00

Member

can you verify that changing this from '-(\d{8}).' to "-(\d{8})." doesn't cause any problems?

can you verify that changing this from `'-(\d{8}).'` to `"-(\d{8})."` doesn't cause any problems?

sayanchowdhury commented

2019-02-28 18:53:51 +00:00

Author

Contributor

1 new commit added

fixup! Fix the clean ami script to remove the amis based on date

**1 new commit added** * ``fixup! Fix the clean ami script to remove the amis based on date``

mohanboddu commented

2019-02-28 19:17:37 +00:00

Contributor

Why is this seperated? Cant it be joined in the for loop below?

mohanboddu commented

2019-02-28 19:22:34 +00:00

Contributor

Use elif since same arch wont be repeated and less checks 😄

Use elif since same arch wont be repeated and less checks :smile:

mohanboddu commented

2019-02-28 19:22:43 +00:00

Contributor

Same as above

dustymabe commented

2019-02-28 19:29:21 +00:00

Member

these changes LGTM - can you address mohan's comments?

mohanboddu commented

2019-02-28 19:32:03 +00:00

Contributor

Same as the one that I asked above, why separated, rather than being the for loop below?

mohanboddu commented

2019-02-28 19:41:29 +00:00

Contributor

I have no idea on how delete amis and change ami permissions work, but other than that and the above comments I made, its LGTM.

sayanchowdhury commented

2019-02-28 19:41:46 +00:00

Author

Contributor

No because the first query returns the number of pages. which is utilized in the next loop

sayanchowdhury commented

2019-02-28 19:44:25 +00:00

Author

Contributor

1 new commit added

fixup! Fix the clean ami script to remove the amis based on date

**1 new commit added** * ``fixup! Fix the clean ami script to remove the amis based on date``

dustymabe commented

2019-02-28 19:55:44 +00:00

Member

LGTM. Let's squash and merge and then can you:

do a test against dev account
do a dry-run against prod account and share output with us in IRC?

LGTM. Let's squash and merge and then can you: 1. do a test against dev account 2. do a dry-run against prod account and share output with us in IRC?

sayanchowdhury commented

2019-02-28 19:56:59 +00:00

Author

Contributor

rebased onto 44e1c85878

rebased onto 44e1c85878d1834fcc423fa82d5d56ca4e6abfe8

mohanboddu commented

2019-02-28 19:59:28 +00:00

Contributor

Pull-Request has been merged by mohanboddu

Rows
Columns

Fix the clean ami script to remove the amis based on date #8146