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.

Sign up for GitHub

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

Jump to bottom

root command to list unverified upstream sources #148

Merged
merged 1 commit into from
Jan 17, 2025
Merged

root command to list unverified upstream sources #148

merged 1 commit into from
Jan 17, 2025

Conversation

manishk-arista
Copy link
Contributor

@manishk-arista manishk-arista commented Dec 13, 2024 •
edited
Loading

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.

manith-arista
manith-arista requested changes Dec 16, 2024
View reviewed changes
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
manith-arista
manith-arista requested changes Dec 18, 2024
View reviewed changes
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
mkisielewski-arista
mkisielewski-arista requested changes Dec 18, 2024
View reviewed changes
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.

aajith-arista
aajith-arista reviewed Dec 19, 2024
View reviewed 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
aajith-arista
aajith-arista reviewed Dec 20, 2024
View reviewed changes
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
View reviewed changes
manith-arista
manith-arista reviewed Dec 30, 2024
View reviewed changes
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
manith-arista
manith-arista reviewed Jan 10, 2025
View reviewed changes
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
View reviewed changes
@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
@manishk-arista manishk-arista force-pushed the manishk-list-unverifired-sources-rootCmd branch from 3d85430 to 0b47bf8 Compare January 13, 2025 06:20
manith-arista
manith-arista previously approved these changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@manith-arista manith-arista left a comment

Choose a reason for hiding this comment

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

Ensure rebase is done before merging

mkisielewski-arista
mkisielewski-arista previously approved these changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@mkisielewski-arista mkisielewski-arista left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks awesome, +1.

BTW
I noticed that some of the new files have 2022 and others 2023 in the topmost comment. Of course it's not a problem, but I think it'd be better to have this an actual year of the file creation, or make it consistent throughout the project.

@manishk-arista manishk-arista force-pushed the manishk-list-unverifired-sources-rootCmd branch from 0b47bf8 to 40e6ef1 Compare January 16, 2025 12:31
@manishk-arista
add root command to list unverified upstream sources
ca9b0e0 
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.

   impl/testData/unverified-src/eext.yaml
@manishk-arista manishk-arista force-pushed the manishk-list-unverifired-sources-rootCmd branch from 40e6ef1 to ca9b0e0 Compare January 16, 2025 12:38
@manishk-arista manishk-arista merged commit 75612a9 into main Jan 17, 2025
1 check passed
@manishk-arista manishk-arista deleted the manishk-list-unverifired-sources-rootCmd branch January 17, 2025 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manith-arista manith-arista manith-arista approved these changes

@aajith-arista aajith-arista Awaiting requested review from aajith-arista

@mkisielewski-arista mkisielewski-arista Awaiting requested review from mkisielewski-arista

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@manishk-arista @manith-arista @aajith-arista @mkisielewski-arista