feature: import targets #195

kairoaraujo · 2023-02-08T12:15:11Z

Implements the feature import target rstuf admin import-targets.

This feature gives the RSTUF administrator the functionality to import a large number of existent targets. It helps to roll out and deploy the RSTUF in existing repositories.

This feature is detailed and explained at these links

Some changes in the ceremony was done to reuse in the code:

The ceremony._check_server was converted to helpers.api_client.get_headers
The ceremony._bootstrap_state was converted to helpers.api_client.task_status

Added 100% coverage to helpsers.api_client

Signed-off-by: Kairo de Araujo kdearaujo@vmware.com

Implements the feature import target `rstuf admin import-targets`. This feature gives the RSTUF administrator the functionality to import a large number of existent targets. It helps to roll out and deploy the RSTUF in existing repositories. This feature is detailed and explained at these links - repository-service-tuf/repository-service-tuf#188 - BDD Feature: repository-service-tuf/repository-service-tuf#218 Some changes in the ceremony was done to reuse in the code: - The `ceremony._check_server` was converted to `helpers.api_client.get_headers` - The `ceremony._bootstrap_state` was converted to `helpers.api_client.task_status` Added 100% coverage to `helpsers.api_client` Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

MVrachev · 2023-02-09T11:46:15Z

This feature implements #39, right?
Because in your comment here repository-service-tuf/repository-service-tuf#188 (comment) you described all of the steps required to finish #188 and two remain: one is documentation and the other is importing targets in the CLI.

kairoaraujo · 2023-02-09T12:25:34Z

This feature implements #39, right?

It partially implements #39.
The #39 is about adding targets add-targets. This feature should be an interface for the REST API
It ended up a bit confusing.

Should we create a specific issue for it? Should I also implement add-targets to this PR?

Because in your comment here vmware/repository-service-tuf#188 (comment) you described all of the steps required to finish #188 and two remain: one is documentation and the other is importing targets in the CLI.

What I mention is that "the feature is described in repository-service-tuf/repository-service-tuf#188" and on the BDD.

MVrachev · 2023-02-09T12:52:48Z

This feature implements #39, right?

It partially implements #39. The #39 is about adding targets add-targets. This feature should be an interface for the REST API It ended up a bit confusing.

I see what you did now.
I was confused about the state of #39 as in your comment #39 (comment) you laid down only two todo items for #39, but it seems that there are more.

Should we create a specific issue for it? Should I also implement add-targets to this PR?

I don't think it will be necerry to create a new issue now when we have a pr, but could you update #39 (comment) comment with the remaining steps (mentioning this step of importing targets) and their status?

Because in your comment here vmware/repository-service-tuf#188 (comment) you described all of the steps required to finish #188 and two remain: one is documentation and the other is importing targets in the CLI.

What I mention is that "the feature is described in vmware/repository-service-tuf#188" and on the BDD.

Yes, got it.

MVrachev

I reviewed the core code without the tests yet.
I have too many questions/comments on it.

Overall the work is big. It's visible it caused you a great amount of time an effort.

Besides my comments: do you plan on adding a test file with tests specifically testing
repository_service_tuf/cli/admin/import_targets.py?

Pipfile

repository_service_tuf/helpers/api_client.py

MVrachev · 2023-02-09T13:20:25Z

repository_service_tuf/helpers/api_client.py

+            response = request_server(
+                server, URL.bootstrap.value, Methods.get, headers=headers
+            )
+            if response.status_code != 200:


In the previous implementation when this was the _check_server function inside ceremony.py you also checked in the response if bootstrap is true.
I see that in ceremony.py before calling get_headers you check the bootstrap status and it is not required but inside the new file import_targets.py you don't check the bootstrap status before calling get_headers.
Is this missed or it's deliberate?

Yes, the import-targets requires the bootstrap done. I will add the check.

Now I am looking into this code again and I am wondering: why do you add a separate request_server call in lines 159-161 in import_targets.py just to complete the bootstrap check when you can make it there?
What do you save by not doing it here directly?
You already made the request_server call to ask about the bootstrap status, not sure why you don't parse the response here directly...
Is it to not repeat the bootstrap check when you call get_headers from the ceremony.py file where you already did it?

get_headers returns only the headers.
The call used by get_headers to the bootstrap is to validate the token itself as the CLI token has all required scopes.

The bootstrap check from the ceremony and import-targets is the same data but has a different check.

ceremony the expect a bootstrap not completed.

import-targets expect a bootstrap completed.

As I have mentioned in my comment here 82b64ab#r1106678265 we can continue this discussion in a separate issue as there are already lots of changes.

repository_service_tuf/cli/admin/import_targets.py

repository_service_tuf/helpers/api_client.py

Co-authored-by: Martin Vrachev <martin.vrachev@gmail.com>

The optional dependencies `psycopg2` and `sqlalchemy` are optional. This is required only when the user aim to use the `import-targets` feature. Add to documentation the new feature Add details about the usage and CSV Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

- add typing notation - make the `_check_csv_files` more explicity Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

KAUTH · 2023-02-12T23:30:04Z

I will have a look tomorrow and review as well 👍

Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

- Added some small refactoring in the implementation - 100% coverage for the UT - Fix some comments from the review Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

kairoaraujo · 2023-02-13T13:34:58Z

@MVrachev, it is ready for another review.

MVrachev

Reviewed everything except the test_api_client.py file.
Still, I decided to submit my review as I have multiple questions and comments.

docs/source/guide/index.rst

tests/unit/cli/admin/test_ceremony.py

MVrachev · 2023-02-13T15:48:34Z

repository_service_tuf/helpers/api_client.py

+            response = request_server(
+                server, URL.bootstrap.value, Methods.get, headers=headers
+            )
+            if response.status_code != 200:


Now I am looking into this code again and I am wondering: why do you add a separate request_server call in lines 159-161 in import_targets.py just to complete the bootstrap check when you can make it there?
What do you save by not doing it here directly?
You already made the request_server call to ask about the bootstrap status, not sure why you don't parse the response here directly...
Is it to not repeat the bootstrap check when you call get_headers from the ceremony.py file where you already did it?

repository_service_tuf/helpers/api_client.py

tests/unit/cli/admin/test_import_targets.py

Co-authored-by: Martin Vrachev <martin.vrachev@gmail.com>

KAUTH · 2023-02-13T22:22:15Z

repository_service_tuf/helpers/api_client.py

@@ -69,6 +74,94 @@ def is_logged(server: str, token: str):
            return Login(state=True, data=data)

    else:
-        click.ClickException(
-            f"Error {response.status_code} {response.json()['detail']}"
+        raise click.ClickException(


I think is_logged does not work as expected in all cases.
A GET api/v1/token/ request may also return a 200 response with a body of:

{ "detail": { "error": "Failed to validate token" } }

See here. In this case, data is None and data.get("expired") will throw an AttributeError

I spotted it here, but if you validate it I can open a bug. (BTW quick fix: data = response.json().get("data", {}))

On the other hand, we are always validating a token with the same token in the headers, so I assume if the token is invalid it won't be because it can't be decoded... so it might be fine this way. I will not delete it, but I believe you can ignore my comment.

I spotted it here, but if you validate it I can open a bug. (BTW quick fix: data = response.json().get("data", {}))

That's a good spot. I'm implementing the quick fix.

On the other hand, we are always validating a token with the same token in the headers, so I assume if the token is invalid it won't be because it can't be decoded... so it might be fine this way. I will not delete it, but I believe you can ignore my comment.

Maybe we can have a specific issue for this part to implement or remove this check.
Can you file the issue giving the background?

@KAUTH when you open it could please link it here for future reference?
Also, a good catch indeed. :))

I opened this issue

Token is validated with same token, is it necessary? #198

repository_service_tuf/cli/admin/import_targets.py

repository_service_tuf/helpers/api_client.py

- move parameters to use double dash `--{names}` - remove duplicate check that is done by `is_logged` - fix tests Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

- fix `is_logged` bug in case of no data with 200 status code - fix typing for the `task_status` response - fix typo/wording for task status Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

tests/unit/helpers/test_api_client.py

- remove login keyword parameters from lambda Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

MVrachev

I see only one place that requires change and I have described it in my comment:
#195 (comment)

Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

codecov-commenter · 2023-02-15T13:19:50Z

Codecov Report

Base: 84.33% // Head: 90.52% // Increases project coverage by +6.18% 🎉

Coverage data is based on head (bc9f82b) compared to base (913e1d8).
Patch coverage: 99.25% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #195      +/-   ##
==========================================
+ Coverage   84.33%   90.52%   +6.18%     
==========================================
  Files           9       10       +1     
  Lines         549      633      +84     
==========================================
+ Hits          463      573     +110     
+ Misses         86       60      -26

Impacted Files	Coverage Δ
repository_service_tuf/cli/admin/ceremony.py	`85.71% <66.66%> (+3.33%)`	⬆️
repository_service_tuf/cli/admin/import_targets.py	`100.00% <100.00%> (ø)`
repository_service_tuf/helpers/api_client.py	`100.00% <100.00%> (+29.72%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

MVrachev

Great job @kairoaraujo!
LGTM from me, but will wait for another day in hope that @KAUTH can review it again.

KAUTH

I had a look at the tests as well, LGTM.
A big PR, nice work guys!

MVrachev · 2023-02-16T07:36:37Z

With two approvals how could you not merge this :))

kairoaraujo requested review from KAUTH and MVrachev February 8, 2023 12:15

kairoaraujo mentioned this pull request Feb 8, 2023

enhancement: publish_targets return the final task. repository-service-tuf/repository-service-tuf-worker#204

Closed

kairoaraujo mentioned this pull request Feb 9, 2023

Task: CLI artifact add command rstuf artifact add #39

Open

MVrachev reviewed Feb 9, 2023

View reviewed changes

kairoaraujo and others added 4 commits February 10, 2023 09:00

Fix the sentences

6ee26ce

Co-authored-by: Martin Vrachev <martin.vrachev@gmail.com>

Add typing notation

f348a57

- add typing notation - make the `_check_csv_files` more explicity Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

fix tests for changed commited to the ceremony

398e301

Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

Kairo de Araujo added 4 commits February 13, 2023 14:13

fix the bootstrap check for ceremony

80d550b

Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

add unit tests for import-targets

82b64ab

- Added some small refactoring in the implementation - 100% coverage for the UT - Fix some comments from the review Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

Removed unused Mock from test_import_targets

087a1d0

Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

fix linting

73b0c9c

Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

MVrachev requested changes Feb 13, 2023

View reviewed changes

kairoaraujo and others added 2 commits February 13, 2023 18:40

Apply suggestions from code review

96e67be

Co-authored-by: Martin Vrachev <martin.vrachev@gmail.com>

Add missing asserts

1320eb5

Co-authored-by: Martin Vrachev <martin.vrachev@gmail.com>

kairoaraujo mentioned this pull request Feb 13, 2023

Refactoring enum types in api_client.py #197

Closed

KAUTH reviewed Feb 13, 2023

View reviewed changes

repository_service_tuf/cli/admin/import_targets.py Outdated Show resolved Hide resolved

KAUTH reviewed Feb 13, 2023

View reviewed changes

repository_service_tuf/cli/admin/import_targets.py Show resolved Hide resolved

KAUTH reviewed Feb 13, 2023

View reviewed changes

repository_service_tuf/cli/admin/import_targets.py Outdated Show resolved Hide resolved

KAUTH reviewed Feb 13, 2023

View reviewed changes

repository_service_tuf/helpers/api_client.py Outdated Show resolved Hide resolved

KAUTH reviewed Feb 13, 2023

View reviewed changes

repository_service_tuf/helpers/api_client.py Outdated Show resolved Hide resolved

fix import-targets params, remove duplicate checks

69283e3

- move parameters to use double dash `--{names}` - remove duplicate check that is done by `is_logged` - fix tests Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

fix bug in is_logged, typing and typo

353f44b

- fix `is_logged` bug in case of no data with 200 status code - fix typing for the `task_status` response - fix typo/wording for task status Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

This was referenced Feb 15, 2023

Token is validated with same token, is it necessary? #198

Open

Refactoring the bootstrap status check #199

Open

kairoaraujo requested review from MVrachev and KAUTH February 15, 2023 07:53

MVrachev reviewed Feb 15, 2023

View reviewed changes

remove **kw from api_client.Login lambdas

ec6c599

- remove login keyword parameters from lambda Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

kairoaraujo requested a review from MVrachev February 15, 2023 09:44

MVrachev requested changes Feb 15, 2023

View reviewed changes

update documentation using -- to the parameters

bc9f82b

Signed-off-by: Kairo de Araujo <kdearaujo@vmware.com>

kairoaraujo requested a review from MVrachev February 15, 2023 13:15

MVrachev approved these changes Feb 15, 2023

View reviewed changes

KAUTH approved these changes Feb 15, 2023

View reviewed changes

MVrachev merged commit 6282adc into repository-service-tuf:main Feb 16, 2023

kairoaraujo mentioned this pull request Feb 21, 2023

Bug: optional dependencies (sqlalchemy and psycopg2) #204

Closed

1 task

MVrachev mentioned this pull request Feb 27, 2023

Task: Document how to import existent targets to RSTUF repository-service-tuf/repository-service-tuf#226

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: import targets #195

feature: import targets #195

kairoaraujo commented Feb 8, 2023

MVrachev commented Feb 9, 2023

kairoaraujo commented Feb 9, 2023

MVrachev commented Feb 9, 2023

MVrachev left a comment

MVrachev Feb 9, 2023

kairoaraujo Feb 10, 2023

MVrachev Feb 13, 2023

kairoaraujo Feb 13, 2023

MVrachev Feb 15, 2023

KAUTH commented Feb 12, 2023

kairoaraujo commented Feb 13, 2023

MVrachev left a comment

MVrachev Feb 13, 2023

KAUTH Feb 13, 2023 •

edited

kairoaraujo Feb 14, 2023

MVrachev Feb 15, 2023 •

edited

kairoaraujo Feb 15, 2023

MVrachev left a comment

codecov-commenter commented Feb 15, 2023

MVrachev left a comment

KAUTH left a comment

MVrachev commented Feb 16, 2023

feature: import targets #195

feature: import targets #195

Conversation

kairoaraujo commented Feb 8, 2023

MVrachev commented Feb 9, 2023

kairoaraujo commented Feb 9, 2023

MVrachev commented Feb 9, 2023

MVrachev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KAUTH commented Feb 12, 2023

kairoaraujo commented Feb 13, 2023

MVrachev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KAUTH Feb 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MVrachev Feb 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MVrachev left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 15, 2023

Codecov Report

MVrachev left a comment

Choose a reason for hiding this comment

KAUTH left a comment

Choose a reason for hiding this comment

MVrachev commented Feb 16, 2023

KAUTH Feb 13, 2023 •

edited

MVrachev Feb 15, 2023 •

edited