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

chore: add lockfile to PackageDescriptor #282

Merged
merged 4 commits into from
Aug 8, 2023
Merged

chore: add lockfile to PackageDescriptor #282

merged 4 commits into from
Aug 8, 2023

Conversation

ejortega
Copy link
Contributor

@ejortega ejortega commented Jul 25, 2023 •
edited by maxrake
Loading

Updates the package descriptor with an additional lockfile field.

Closes #279

Checklist

  • Does this PR have an associated issue (i.e., closes #<issueNum> in description above)?
  • Have you ensured that you have met the expected acceptance criteria?
  • Have you created sufficient tests?
  • Have you updated all affected documentation?

@ejortega ejortega linked an issue Jul 26, 2023 that may be closed by this pull request
Capture package lockfile paths #279
Closed
2 tasks
@ejortega ejortega marked this pull request as ready for review July 28, 2023 21:04
@ejortega ejortega requested a review from a team as a code owner July 28, 2023 21:04
@ejortega ejortega force-pushed the add-lockfile-path branch 3 times, most recently from ce3ac94 to 4eaea1e Compare July 28, 2023 21:35
@ejortega ejortega changed the title feat: add lockfile_path to PackageDescriptor feat: add lockfile to PackageDescriptor Jul 28, 2023
@ejortega ejortega force-pushed the add-lockfile-path branch 3 times, most recently from f95483a to 5b1db37 Compare July 28, 2023 22:16
@maxrake maxrake changed the title feat: add lockfile to PackageDescriptor chore: add lockfile to PackageDescriptor Aug 8, 2023
maxrake
maxrake approved these changes Aug 8, 2023
View reviewed changes
Copy link
Contributor

@maxrake maxrake 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.

The title was changed from feat: to chore: since the real feature is controlled by changes in CLI and API. That can be changed back if you like, but then the rest of the title and description text (which gets used in the squash commit) should describe a feature that is meaningful to users.

Has the CLI/API code been updated already to account for the 2nd Acceptance Criteria in #279?:

The CI should show the lockfile path on the comments it creates.

The unit test was a nice addition and helped to learn more about mocking with unittest.mock.

LGTM.

@ejortega
Copy link
Contributor Author

ejortega commented Aug 8, 2023 •
edited
Loading

The title was changed from feat: to chore: since the real feature is controlled by changes in CLI and API. That can be changed back if you like, but then the rest of the title and description text (which gets used in the squash commit) should describe a feature that is meaningful to users.

No problem, I wasn't quite sure but that makes sense.

Has the CLI/API code been updated already to account for the 2nd Acceptance Criteria in #279?:

API has been updated and running. CLI needs phylum-dev/cli#1172 to get approved and merged for the cli to output the lockfile paths. I believe there will be a release shortly following that PR approval.

However, the API still accepts submissions without the lockfile paths and just defaults the paths to null.

I'm not opposed to waiting until the next CLI release to merge this PR.

@maxrake
Copy link
Contributor

maxrake commented Aug 8, 2023 •
edited
Loading

@ejortega I think this PR can be merged now...no need to wait. This is mostly because the change here was made to work both with and without the lockfile attribute of a PackageDescriptor, allowing it to be released at any time.

Plus, the comment text is controlled by code in the api repo now (to ensure consistency with the GitHub App) so that should just work. If anything, that second acceptance criteria in #279 could be removed since the PR comment content is no longer produced by phylum-ci.

@ejortega ejortega merged commit 042ba82 into main Aug 8, 2023
8 checks passed
@ejortega ejortega deleted the add-lockfile-path branch August 8, 2023 16:23
maxrake added a commit that referenced this pull request Sep 6, 2023
@maxrake
fix: incorrect new dependency logic
a51442e 
This change fixes a problem where _any_ change in a lockfile results in
the finding that _all_ dependencies from the lockfile are new. This was
introduced in #282, when the `PackageDescriptor` dataclass added the
`lockfile` attribute.

The issue is that the `Lockfile` class uses the `PackageDescriptor`
dataclass to make comparisons and compute unique sets of dependencies.
The `get_previous_lockfile_packages` function works by writing the
previous lockfile object to a temporary file and then parsing that file
with the `phylum parse` command. The temporary file will never have the
same name as the current lockfile and so the `lockfile` attribute will
be different for _all_ dependencies in the given lockfile.

Making this change ensures the `lockfile` attribute of the dataclass is
not included when generating equality and comparison methods. There is
not a problem with this exclusion because the `Lockfile` class still
only operates on one lockfile at a time and keeps track of the
lockfile's path separately, in the `path` property.
@maxrake maxrake mentioned this pull request Sep 6, 2023
Merged
maxrake added a commit that referenced this pull request Sep 7, 2023
@maxrake
fix: incorrect new dependency logic (#304)
b447b46 
This change fixes a problem where _any_ change in a lockfile results in
the finding that _all_ dependencies from the lockfile are new. This was
introduced in #282, when the `PackageDescriptor` dataclass added the
`lockfile` attribute.

The issue is that the `Lockfile` class uses the `PackageDescriptor`
dataclass to make comparisons and compute unique sets of dependencies.
The `get_previous_lockfile_packages` function works by writing the
previous lockfile object to a temporary file and then parsing that file
with the `phylum parse` command. The temporary file will never have the
same name as the current lockfile and so the `lockfile` attribute will
be different for _all_ dependencies in the given lockfile.

Making this change ensures the `lockfile` attribute of the dataclass is
not included when generating equality and comparison methods. There is
not a problem with this exclusion because the `Lockfile` class still
only operates on one lockfile at a time and keeps track of the
lockfile's path separately, in the `path` property.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxrake maxrake maxrake approved these changes

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

Successfully merging this pull request may close these issues.

Capture package lockfile paths
2 participants
@ejortega @maxrake