Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

root command to list unverified upstream sources #148

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

manishk-arista
Copy link
Contributor

@manishk-arista manishk-arista commented Dec 13, 2024

New root command is designed to list all upstream sources with the skip-check flag set to true.

  • If -p <package> is specified, it lists unverified sources for the specified package.
  • If not forund in repo, error is thrown
    The output is written to stdout
    This will enable better tracking of unverified sources.

impl/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources.go Outdated Show resolved Hide resolved
@manishk-arista manishk-arista force-pushed the manishk-list-unverifired-sources-rootCmd branch 2 times, most recently from d9a0b02 to 96ebecb Compare December 16, 2024 14:45
impl/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources_test.go Outdated Show resolved Hide resolved
cmd/list_unverified_sources.go Outdated Show resolved Hide resolved
@manishk-arista manishk-arista force-pushed the manishk-list-unverifired-sources-rootCmd branch from 96ebecb to 721253d Compare December 18, 2024 07:39
Copy link
Contributor

@mkisielewski-arista mkisielewski-arista left a comment

Choose a reason for hiding this comment

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

Some questions, suggestions posted below.
Overall the function introduced here does some logic and writes a file in the end. Which makes the testing of it difficult. If the logic would be separate from writing the tests could be easier to write, stateless, and would not need to be run in a container.

impl/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources.go Show resolved Hide resolved
impl/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources_test.go Outdated Show resolved Hide resolved
impl/list_unverified_sources_test.go Outdated Show resolved Hide resolved
impl/list_unverified_sources_test.go Show resolved Hide resolved
@manishk-arista manishk-arista force-pushed the manishk-list-unverifired-sources-rootCmd branch from 721253d to 59cdbba Compare December 19, 2024 05:28
@manishk-arista
Copy link
Contributor Author

Some questions, suggestions posted below. Overall the function introduced here does some logic and writes a file in the end. Which makes the testing of it difficult. If the logic would be separate from writing the tests could be easier to write, stateless, and would not need to be run in a container.

I have made some previously discussed changes to code. Now I have removed some part of code and made changes to existing ones. I think most of the review comments are addressed in these new changes.

cmd/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources_test.go Outdated Show resolved Hide resolved
impl/list_unverified_sources.go Outdated Show resolved Hide resolved
@manishk-arista manishk-arista force-pushed the manishk-list-unverifired-sources-rootCmd branch 2 times, most recently from 2b2c92b to 8e75411 Compare December 19, 2024 09:22
@manishk-arista manishk-arista changed the title add root command to list unverified upstream sources root command to list unverified upstream sources Dec 20, 2024
cmd/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources.go Show resolved Hide resolved
impl/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources_test.go Outdated Show resolved Hide resolved
@manishk-arista manishk-arista force-pushed the manishk-list-unverifired-sources-rootCmd branch from 8e75411 to 4d820ec Compare December 23, 2024 06:19
aajith-arista
aajith-arista previously approved these changes Dec 23, 2024
cmd/list_unverified_sources.go Show resolved Hide resolved
cmd/list_unverified_sources.go Show resolved Hide resolved
impl/list_unverified_sources.go Show resolved Hide resolved
impl/list_unverified_sources.go Outdated Show resolved Hide resolved
impl/list_unverified_sources_test.go Outdated Show resolved Hide resolved
@manishk-arista manishk-arista force-pushed the manishk-list-unverifired-sources-rootCmd branch from 4d820ec to 246ddfc Compare December 30, 2024 13:50
@manishk-arista manishk-arista force-pushed the manishk-list-unverifired-sources-rootCmd branch 2 times, most recently from df0bf50 to 595661e Compare January 10, 2025 03:51
@manishk-arista manishk-arista force-pushed the manishk-list-unverifired-sources-rootCmd branch 5 times, most recently from 12695bd to d001770 Compare January 10, 2025 07:15
impl/list_unverified_sources.go Show resolved Hide resolved
impl/list_unverified_sources_test.go Outdated Show resolved Hide resolved
impl/list_unverified_sources_test.go Outdated Show resolved Hide resolved
impl/list_unverified_sources_test.go Outdated Show resolved Hide resolved
@manishk-arista manishk-arista force-pushed the manishk-list-unverifired-sources-rootCmd branch from d001770 to c4f8572 Compare January 10, 2025 09:21
manith-arista
manith-arista previously approved these changes Jan 10, 2025
@manith-arista
Copy link
Contributor

Rebase the PR with latest main branch changes

@manishk-arista manishk-arista force-pushed the manishk-list-unverifired-sources-rootCmd branch from c4f8572 to 3d85430 Compare January 13, 2025 06:01
new root command is designed to list all upstream
sources with the `skip-check` flag set to `true`.
- If `-p <package>` is specified, it lists unverified
sources for the specified package.
- If <package> not forund in repo, error is thrown

The output is written to stdout

This will enable better tracking of unverified sources.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants