Skip to content

Conversation

BartoszBlizniak
Copy link
Member

@BartoszBlizniak BartoszBlizniak commented Sep 25, 2024

This PR includes the suggestion/contribution from PR 171 (Issue 170)

  • Added --show-all flag to all supported pagination elements (Repo, Package, Upstream, Policy)
  • Updated unit tests to reflect the change
  • Include code from PR 168 that uploads the ZipApp to the repo
  • Added black formatting step to Contribution docs
  • Added slug_perm to documentation for dependency command for clarity


print_vulnerability_policies(policies)

click.echo()
Copy link
Member

Choose a reason for hiding this comment

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

does the newline break JSON parsing if you use JSON output?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unresolved this one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

☝️ Still need an answer here

@BartoszBlizniak BartoszBlizniak force-pushed the ceng-355-httpsgithubcomcloudsmith-iocloudsmith-clipull171 branch from cf3ca3b to c79c929 Compare October 16, 2024 16:18
current_page = page
while True:
page_results, page_info = api_function(
page=current_page, page_size=page_size, **kwargs
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're in the "show all" block, we should probably disregard the page_size that was passed into the function and hard code {{whatever the maximum page_size is}} to minimise chattiness.

Do we have a MAX_PAGE_SIZE constant or equivalent in this project already that we might reuse? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

☝️ We should do this - reach out if you need more 👀


print_vulnerability_policies(policies)

click.echo()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unresolved this one.

Copy link

Code Climate has analyzed commit ce41ea2 and detected 10 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 8
Duplication 2

The test coverage on the diff in this pull request is 87.5% (50% is the threshold).

This pull request will bring the total coverage in the repository to 73.4% (1.1% change).

View more on Code Climate.

Comment on lines 158 to 168
def validate_show_all(ctx, param, value):
"""Ensure that --show-all is not used with --page (-p) or --page-size (-l)."""
if value:
# Check if either page or page_size parameters were provided, regardless of value
if any(param in ctx.params for param in ["page", "page_size"]):
raise click.UsageError(
"The --show-all option cannot be used with --page (-p) or --page-size (-l) options."
)
return value


Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a test for this please?

Does the order in which the parameters are passed affect the validation?

current_page = page
while True:
page_results, page_info = api_function(
page=current_page, page_size=page_size, **kwargs
Copy link
Collaborator

Choose a reason for hiding this comment

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

☝️ We should do this - reach out if you need more 👀


print_vulnerability_policies(policies)

click.echo()
Copy link
Collaborator

Choose a reason for hiding this comment

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

☝️ Still need an answer here

…71' of github.com:cloudsmith-io/cloudsmith-cli into ceng-355-httpsgithubcomcloudsmith-iocloudsmith-clipull171
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants