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

Add rpm name search and integration testing #1

Merged
merged 3 commits into from
Jan 3, 2024

Conversation

rverdile
Copy link
Collaborator

@rverdile rverdile commented Dec 8, 2023

Summary

Adds

  • An exported interface, Tangy, that that includes the method RpmRepositoryVersionPackageSearch(), to search RPMs by name given a list of repository hrefs.
  • Tangy includes database and logging configuration with zerolog.
  • 3 internal (not exported) packages used for testing/development purposes.
  • Make commands to start up a pulp server with podman-compose.
  • A CI integration test for searching RPMs by name
  • A CI test for linting
  • A README with instructions on to how use this repository

@rverdile rverdile force-pushed the rpm-search branch 11 times, most recently from d4b58fe to cbee8be Compare December 11, 2023 18:37
@rverdile rverdile changed the title changeme Add rpm name search and integration testing Dec 11, 2023
@rverdile rverdile marked this pull request as ready for review December 11, 2023 18:41
pkg/tangy/rpm.go Outdated
SELECT DISTINCT ON (rp.name) rp.name, rp.summary
FROM core_repositoryversion crv
INNER JOIN core_repository cr on crv.repository_id = cr.pulp_id
INNER JOIN core_repositorycontent crc on crv.pulp_id = crc.version_added_id
Copy link
Member

Choose a reason for hiding this comment

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

would version_added cause this to only be returned for the version that actually added the RPM?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i think it would, so I changed it to

SELECT DISTINCT ON (rp.name) rp.name, rp.summary
		FROM core_repositoryversion crv
		INNER JOIN core_repository cr on crv.repository_id = cr.pulp_id
		INNER JOIN core_repositorycontent crc on cr.pulp_id = crc.repository_id
		INNER JOIN core_content cc on crc.content_id = cc.pulp_id
		INNER JOIN rpm_package rp on cc.pulp_id = rp.content_ptr_id
		WHERE CONCAT(crv.repository_id, '/', crv.number) = ANY ($1)
		AND rp.name ILIKE CONCAT( '%', $2::text, '%')

Copy link
Member

Choose a reason for hiding this comment

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

instead of trying to test manually, i updated your test to simulate re-syncing a repository to produce two different versions: https://gist.github.com/jlsherrill/5008aaee911029e74edb22dc014cd245

It all worked except when i search the first version after having created the second version, i seem to be getting the second version's results?

@rverdile rverdile force-pushed the rpm-search branch 3 times, most recently from d70817d to 40d1087 Compare December 13, 2023 20:25
# Configuration options for the pulp server
server:
url: "http://localhost:8087"
username: "pulp"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
username: "pulp"
username: "admin"

pkg/tangy/rpm.go Outdated
Summary string
}

func (t *tangyImpl) RpmRepositoryVersionPackageSearch(ctx context.Context, hrefs []string, search string) ([]RpmPackageSearch, error) {
Copy link
Member

Choose a reason for hiding this comment

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

i think we want this to take a limit, so we can expose that limit to the user like we do for rpms

if dbConfig.PoolLimit == 0 {
dbConfig.PoolLimit = DefaultMaxPoolLimit
}
pxConfig.MaxConns = int32(dbConfig.PoolLimit)
Copy link
Member

Choose a reason for hiding this comment

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

we should probably have a method that would allow the application to close/release the connection pool

@jlsherrill
Copy link
Member

Also, would it make sense to include a Mock of the interface here? Instead of in our application.

fix query
add limit
add close method
add new test cases
add mock

Co-authored-by: Justin Sherrill <[email protected]>
@rverdile
Copy link
Collaborator Author

updated


query += "UNION"
}
rows, err := conn.Query(context.Background(), query)
Copy link
Member

Choose a reason for hiding this comment

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

if you accidentally pass in an empty list, you end up getting a query error, it might be worth catching and just returning an empty list

Copy link
Member

Choose a reason for hiding this comment

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

i think this is the last suggestion, otherwise looking good!

@rverdile rverdile merged commit e191e0e into content-services:main Jan 3, 2024
2 checks passed
@rverdile rverdile deleted the rpm-search branch January 3, 2024 14:22
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.

2 participants