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

Calculate libyear #112

Merged
merged 87 commits into from
Jul 13, 2022
Merged

Calculate libyear #112

merged 87 commits into from
Jul 13, 2022

Conversation

edwinkortman
Copy link
Contributor

@edwinkortman edwinkortman commented Jun 17, 2022

Fixes #97

Technically this issue isn't done yet, but I figured I'd give it for review anyway.
What I've added is a few things to make calculating libyear possible for a given package url. The idea is that the Package URL comes from the Software Bill of Materials (CycloneDX file). With this package url we can use the dependency manager repository to query info about the package, specifically where it's versioned. Then we can find out when the tags have been published to get a release date for a specific version. There's a bit of a diagram in the issue #97 which I hope helps clarify things.

Why it's not done is because the repositories don't do anything yet. I'm working on making it work for at least one of them but it's starting to feel like that's growing beyond the scope of this issue, so I'd prefer to split the work here. I'll make issues for the remaining work if this okay with everyone? :-)

Left to do for this PR

  • Make issues for the separate repositories once this is merged and team has agreed on taking that route

Solved
Another thing is that it's failing in Code Climate, but I don't entirely agree with the reason. It's tripping of similar blocks of code which they are but they test something vastly different. My suggestion here is to dial it down a little, if that's possible?

1

[Fact]
    public void It_can_not_instantiate_with_invalid_dependency_manager()
    {
        var caughtException =
            Assert.Throws<ArgumentException>(() =>
                SupportedDependencyManagers.FromString("prettysurethiscanneverwork"));

        Assert.Equal("Invalid dependency manager given 'prettysurethiscanneverwork'", caughtException.Message);
    }

2

[Fact]
    public void Verify_it_throws_exception_when_no_filepath_was_given()
    {
        var caughtException = Assert.Throws<ArgumentException>(() => _readCycloneDxFile.AsPackageURLs(""));

        Assert.Equal("Can not read file, as no file path was given", caughtException.Message);
    }

@edwinkortman edwinkortman self-assigned this Jun 17, 2022
@edwinkortman edwinkortman marked this pull request as ready for review June 17, 2022 16:10
@mscottford
Copy link
Member

Another thing is that it's failing in Code Climate, but I don't entirely agree with the reason. It's tripping of similar blocks of code which they are but they test something vastly different. My suggestion here is to dial it down a little, if that's possible?

That's really strange. I agree that those two methods don't appear similar to me. I think we should adjust the CodeClimate's duplication engine configuration to specify a higher "mass" value.

@edwinkortman
Copy link
Contributor Author

Another thing is that it's failing in Code Climate, but I don't entirely agree with the reason. It's tripping of similar blocks of code which they are but they test something vastly different. My suggestion here is to dial it down a little, if that's possible?

That's really strange. I agree that those two methods don't appear similar to me. I think we should adjust the CodeClimate's duplication engine configuration to specify a higher "mass" value.

Thank you for the suggestion! :D I've added the threshold for similar code and '70' seems to be the sweet spot for this particular bit of similar code.

@mscottford mscottford added the enhancement New feature or request label Jun 21, 2022
@mscottford
Copy link
Member

Why it's not done is because the repositories don't do anything yet. I'm working on making it work for at least one of them but it's starting to feel like that's growing beyond the scope of this issue, so I'd prefer to split the work here. I'll make issues for the remaining work if this okay with everyone? :-)

I think this makes sense for now. The way that I think this should be implemented is to delegate to the freshli-agent-* programs. We should add a retrieve-release-history command to the each of the agent programs for this purpose. I think we can make that command take an option called --list-valid-purls which will output the PURL types that the command is able to retrieve history for. I've added an issue for the freshli-agent-java repository for this purpose.

So I think the process we would follow would be to run agents detect to find all of the agents, and then run each one of them with retrieve-release-history --list-valid-purls to determine the package urls that the agent supports, and then use this information for determining the correct agent that should be called to get the release history of a specific package by running retrieve-release-history package-url for each package that history information is needed for.

I'm assuming this information will make more sense in the issue that we create, but I wanted to write it out here while I'm thinking about it.

Copy link
Member

@mscottford mscottford left a comment

Choose a reason for hiding this comment

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

I've got a couple of suggested changes to make, and I've made a couple of observations that should get moved into other issues.

@edwinkortman edwinkortman force-pushed the calculate-libyear branch 2 times, most recently from af09c11 to 266d530 Compare June 23, 2022 15:51
@edwinkortman
Copy link
Contributor Author

@mscottford I've processed your feedback, quite a few bits have changed. Could you have another look? :-)

It was missing whatever Docker was supposed to do
A few notes:
- This willingly ignores that the newest version of a package may have
  been released before the current version. This is ignored by using
just the absolute number. I did this because I see libyear more as the
'time you had to react to an update counted from'. So when the newer
package was released the clock started ticking, and you didn't do
something for x time.

- It knows what leap years are! There's a surprisingly big difference if
  you ignore leap years.
The repository will find the release date for a given package and
version. It could be that it uses something external like git to
determine the date and tag.

For example: composer

1. find out where code is hosted e.g.
2. Use git to query tags and their publication date
This way we don't have to pass around strings or verify strings. And
it's nicer to read. :D
This service uses the name and version to fetch the release date from a
repository. With these releases date we can calculate libyear
In reality they look very similar but the Package URL is slightly
different. This commit changes the content more, hoping that the point
now comes across
Repository is to generic as a name, especially with another repository
incoming it would get confusing fast.
@mscottford
Copy link
Member

@mscottford I've processed your feedback, quite a few bits have changed. Could you have another look? :-)

You weren't kidding about "quite a few bits". :) I'm digging in now.

Copy link
Member

@mscottford mscottford left a comment

Choose a reason for hiding this comment

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

This looks really good. Thanks for the time that you've put into it. I only have a few small changes that I would like to see implemented. And there are some spike issues that we need to do some investigations.

Much of this review as part of an ensemble session that included @donabp, @CodeMouse92, and @edwinkortman.

ClassData is typically used when data is shared. In this case there was only one test using the data so MemberData makes more sense.
Edwin Kortman added 8 commits July 12, 2022 20:05
The string should be identical in Spanish as we are talking about a metric
The interface is wired in the FreshliServiceBuilder. So if we try to use the Service by calling the class name instead of the Interface it'll thrown an error
This is done, so that we can let the test generate this file. This will make it work as the Agents Reader is just returning static data.
@edwinkortman edwinkortman dismissed mscottford’s stale review July 13, 2022 12:33

We agreed that as soon as the feedback was addressed it was okay to merge. :-)

@edwinkortman edwinkortman merged commit 2025b00 into main Jul 13, 2022
@edwinkortman edwinkortman deleted the calculate-libyear branch July 13, 2022 12:33
@github-actions github-actions bot locked and limited conversation to collaborators Jul 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add compute libyear command
2 participants