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

fix: --skip-archived-repos does not filter correctly #157

Conversation

schmidlidev
Copy link

@schmidlidev schmidlidev commented Jan 3, 2025

Description

Fixes #158

Fixes filtering logic for --skip-archived-repos. This logic was incorrect, causing some archived repos to be present in the result even though --skip-archived-repos was passed.

TODOs

Read the Gruntwork contribution guidelines.

  • Update the docs.
  • Run the relevant tests successfully, including pre-commit checks.
  • Ensure any 3rd party code adheres with our license policy or delete this line if its not applicable.
  • Include release notes. If this PR is backward incompatible, include a migration guide.

Release Notes (draft)

Added / Removed / Updated [X].

Fixed a bug in the filtering logic for skipping archived repos.

Migration Guide

None

@ceschae
Copy link
Contributor

ceschae commented Jan 7, 2025

👋 @schmidlidev thanks for the PR! i'm looking at this today. i'm newly assigned to support of this project so i'm just getting up to speed on what's going on here. but i'm on the case!

// So re-slice the repos list if --skip-archived-repos is passed and the repository is in archived/read-only state
for i, repo := range repos {
// So filter the repos list if --skip-archived-repos is passed and the repository is in archived/read-only state
for _, repo := range repos {
Copy link
Contributor

Choose a reason for hiding this comment

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

if config.SkipArchivedRepos {
	for _, repo := range repos {
		if repo.GetArchived() {
			logger.WithFields(logrus.Fields{
				"Name": repo.GetFullName(),
			}).Debug("Skipping archived repository")

			// Track repos to skip because of archived status for our final run report
			config.Stats.TrackSingle(stats.ReposArchivedSkipped, repo)
		} else {
			reposToAdd = append(reposToAdd, repo)
		}
	}
} else {
	reposToAdd = repos
}	

how about checking for the flag first, cut down on the extra reassignments if config.SkipArchivedRepos == false?

@ceschae
Copy link
Contributor

ceschae commented Jan 8, 2025

@schmidlidev thank you so much for the find here - i can see the logic before was overwriting the fact that skipped repos were skipped in future executions of the loop. nice find! i just had a nit that wasn't your doing, just now that we're looking at it, we can tidy up for more maintainable code.

thanks again!!

@eric-Grunt
Copy link

@schmidlidev Howdy. Just bumping this again in case you missed the message above.

@ceschae ceschae mentioned this pull request Jan 28, 2025
4 tasks
@ceschae
Copy link
Contributor

ceschae commented Jan 28, 2025

because this hasn't been updated in a bit, closing in favor of #169

@ceschae ceschae closed this Jan 28, 2025
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.

--skip-archived-repos does not filter all archived repos
3 participants