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

Method Conflict on repositoryType='pypi' ? #785

Closed
jchaves opened this issue Nov 22, 2021 · 10 comments · Fixed by #893
Closed

Method Conflict on repositoryType='pypi' ? #785

jchaves opened this issue Nov 22, 2021 · 10 comments · Fixed by #893

Comments

@jchaves
Copy link
Contributor

jchaves commented Nov 22, 2021

Without knowing too much about the UniRest lib, it seems that the way we are constructing our rest call on the line above is causing some conflict that causes 'pypi' artifacts to end up stored in Nexus with the file name "application/octet-stream".

It would seem that there is a conflict with the method we are trying to call ( 1 ) and the one that is effectively called ( 2 )
[1]: https://github.com/Kong/unirest-java/blob/838cd1d6bb9c80d2e790d8d36fbcf511a248b016/unirest/src/main/java/kong/unirest/HttpRequestMultiPart.java#L69
[2] : https://github.com/Kong/unirest-java/blob/838cd1d6bb9c80d2e790d8d36fbcf511a248b016/unirest/src/main/java/kong/unirest/HttpRequestMultiPart.java#L94

I'm no expert and am in fact having some difficulties following the flow in NexusService, but I guess the fact that we never define a filename in 'nexusParams' for the 'pypi' repositoryType might have something to do with this?

Maybe for type pypi we should use the more explicit [3]: https://github.com/Kong/unirest-java/blob/838cd1d6bb9c80d2e790d8d36fbcf511a248b016/unirest/src/main/java/kong/unirest/HttpRequestMultiPart.java#L87
?

Any feedback will be appreciated!

@clemensutschig
Copy link
Member

I am lost - we do this according to the nexus doc - I am confused what's wrong if anything?

@jchaves
Copy link
Contributor Author

jchaves commented Nov 23, 2021

Hi again,

yeah, I could definitely have been a bit clearer :) . The situation is that some users are reporting that their python artifacts are being stored in Nexus with the literal filename "application/octet-stream".

I've just found the nexus docs, and it seems pretty straightforward, and it would seem that we are doing it correctly.

However, we are calling a method with parameters of types String , InputStream and String. (I guess the correct type parameters would have been, for our use case, String , InputStream and ContentType

My guess is that, based on that signature, the Unirest lib is preparing a request with 'application/octet-stream' as file name.

Were there some tests made when adding this functionality? I may try making some changes to storeComplextArtifact and doing some tests to see if I can get the desired results.

Meanwhile, as I said, I'm no expert on any of this, so if someone sees that I made any mistake with my assumptions, or something, please comment! :D

@jchaves
Copy link
Contributor Author

jchaves commented Nov 23, 2021

For clarity, I'll make this edit in a separate comment: The actual file name on the nexus instance is 'octet-stream' , not 'application/octet-stream'.

I'm trying to figure out where does it get split. Or maybe, where does it come from. Not sure if nexus splits it on that end, or if it's coming from a different place and all my assumptions are wrong, or what might be happening, really. I'll keep updating after I am able to do some testing.

@clemensutschig
Copy link
Member

clemensutschig commented Nov 23, 2021

@jchaves - we are testing this in an automated test (

)

and in 4.x+ we refactored it ... maybe you want to try there. can you get us the lines from the jenkinsfile here - that produces this .. the uploadToNexusStage

@jchaves
Copy link
Contributor Author

jchaves commented Nov 23, 2021

Sure.
Acording to user, this is the fragment:

stageBuild(context)
odsComponentStageUploadToNexus(
  context,
  [
    distributionFile: 'dist/filename-with-version-numbers-and-stuff.whl',
    repository: 'pypi-reponame',
    repositoryType: 'pypi'
  ]
)

I hope I'll be able to perform some testing during this afternoon, but can't really be sure yet :( . I'll keep updating you

@clemensutschig
Copy link
Member

is this 3.x? or an early master fork? - and can you past the log from the jenkins run (without an internal server names, etc)

@jchaves
Copy link
Contributor Author

jchaves commented Nov 24, 2021

We finally were able to get some testing done, let me try to recap here the results:

  • With a 'clean' 3.x lib, we get the filename 'octet-stream' in the nexus repo
  • With this change we somehow end up with a .zip file (I guess nexus renames it to conform to some standard?)
  • With this other change, .zip again (if I recall correctly)
  • With a 'clean' 4.x, 'octet-stream' again

While we were at it, the user showed me a succesful upload using postman, when we tried generating code for the 'Unirest' library, we saw that the following code would work:

HttpResponse<String> response = Unirest.post("[domain].com/service/rest/v1/components?repository=pypi-[repo]")

  .header("Authorization", "Basic [AUTH data]")

  .header("Cookie", "[COOKIE DATA]")

  .field("file", new File("[PATH TO FILE]/[FILENAME WITH VERSION AS SEEN ON JENKINSFILE].whl"))

  .asString();

The interesting line would be:

.field("file", new File("[PATH TO FILE]/[FILENAME WITH VERSION AS SEEN ON JENKINSFILE].whl"))

so, maybe the easiest path to fixing this would be to stop handling pypi repos as a special case and go back to 'raw' file upload?

I'll try to get some more testing done, to see if I can provide a clean working patch.

Meanwhile, as always, any feedback is welcome :)

@metmajer
Copy link
Member

@jchaves can we close this issue?

@jchaves
Copy link
Contributor Author

jchaves commented Jan 31, 2022

Hi @metmajer,
To be completely honest, I had made a mental note to check on all of this before the holidays break, got busy with other stuff, and then I completely forgot.

There was some conflict waiting for the merge into 4.x [1], I just resolved it and, pending a review from Clemens, it should be ready to merge. Once we do that, everything should be ready to close this issue for good :)

[1] #812

I'll check later today and close it if everything looks fine

@jchaves
Copy link
Contributor Author

jchaves commented Feb 2, 2022

@metmajer @clemensutschig
we still have this PR open, and I don't think I have the permissions to merge and close it:
#812
If you guys wanna take a look and accept it, we could close both the PR and this issue. Having it open is only creating noise at this point, in my opinion.

Thanks, and regards!

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 a pull request may close this issue.

3 participants