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

Force option #4

Merged
merged 7 commits into from
Jun 21, 2021
Merged

Force option #4

merged 7 commits into from
Jun 21, 2021

Conversation

chaws
Copy link
Contributor

@chaws chaws commented Jun 16, 2021

Passing -f/--force will force vidx2pidx to go inside pdsc files cross-checking Vendor, URL, Name and Version against the pdsc tags provided in pidx and vidx files.

Also, by default files are going to be saved in .idxcache folder, but that can be customized by using -c/--cachedir flag

This will be useful when having a whole new struct for
the PdscXML to parse pdsc files.

Signed-off-by: Charles Oliveira <charles.oliveira@linaro.org>
This struct represents the pdsc file. It's used to validate
if the pdsc tags in pidx files really match the actual
pdsc files

Signed-off-by: Charles Oliveira <charles.oliveira@linaro.org>
If CacheDir has a valid name, ReadURL will save all incoming
downloads into it.

Signed-off-by: Charles Oliveira <charles.oliveira@linaro.org>
This patch adds support for checking pdsc URL, Vendor, Name or Version
information against the actual pdsc file.

Setting `PidxXML.force` to true will make AddPdsc download the pdsc file
and validate if what's in the pdsc tag matches the info in the file itself.

When it matches, then things are OK, when it doesn't match, raise an error
and add the pdsc from the file, instead of the tag.

When pdsc files aren not reachable, raise a error saying the reason why, then
adds the pdsc tag as if `.force` was set to False.

Signed-off-by: Charles Oliveira <charles.oliveira@linaro.org>
Allow users to decide when to use force checking pdsc files or not.

The cache dir flag defines a directory to where all downloaded files
are going to be stored.

Signed-off-by: Charles Oliveira <charles.oliveira@linaro.org>
This patch will force the package index generation despite eventual errors.

If the errors are just too much, then generate an empty file, otherwise
generate with whatever it has after recovering from errors.

Signed-off-by: Charles Oliveira <charles.oliveira@linaro.org>
Signed-off-by: Charles Oliveira <charles.oliveira@linaro.org>
cli.go Show resolved Hide resolved
pdsc.go Show resolved Hide resolved

Logger.SetFile(currLogFile)
Logger.SetLevel(currLevel)
ExitOnError(os.RemoveAll(outputFileName))

Choose a reason for hiding this comment

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

This is fine for now, but I would probably just check the error here instead of having a function that blindly checks the error and prints it out. It's possible you may want to add context in the future and wrapping it this way makes it more difficult.

I know all the if err stuff seems very repetitive, but its common Go that everyone understands and works with. It also shows what can happen given an operation failing.

"fmt"
"net/http"
"net/http/httptest"
"testing"

"bou.ke/monkey"

Choose a reason for hiding this comment

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

This dependency has been archived on github. You may need to find a replacement in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have a good replacement for monkey patching?


t.Run("test catch errors", func(t *testing.T) {
errMessage := "Fail to create dirs"
monkey.Patch(os.MkdirAll, func(path string, perm os.FileMode) error {

Choose a reason for hiding this comment

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

I'm not sure I would monkey patch here instead of trying to create a directory in some way that fails that you forsee the user doing. However, I think its fine for now!

@jscook2345
Copy link

LGTM!

@chaws
Copy link
Contributor Author

chaws commented Jun 21, 2021

Thanks for reviewing @jscook2345 ! I'll merge this now, but I left the comments above. I'd like to know your opinion on monkey patching, I didn't find another popular repo yet.

@chaws chaws merged commit ce638e7 into Open-CMSIS-Pack:main Jun 21, 2021
@chaws chaws deleted the force-option branch June 21, 2021 19:08
@chaws chaws mentioned this pull request Jun 21, 2021
@jscook2345
Copy link

I'm not a huge fan of monkey patching, but since you're keeping it to a minimum I think it's fine for now. Most testing libraries would try to use a Mocking framework in this case I think.

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