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

feat: Publish artifacts to maven #946

Merged
merged 2 commits into from
Sep 21, 2024
Merged

Conversation

parthchandra
Copy link
Contributor

@parthchandra parthchandra commented Sep 16, 2024

Which issue does this PR close?

Part of #721

@parthchandra parthchandra marked this pull request as draft September 16, 2024 17:24
@parthchandra parthchandra marked this pull request as ready for review September 19, 2024 21:20
@parthchandra
Copy link
Contributor Author

@andygrove This is ready to be tried out.

@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 34.19%. Comparing base (fa275f1) to head (6141a26).

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #946      +/-   ##
============================================
+ Coverage     34.16%   34.19%   +0.02%     
- Complexity      880      894      +14     
============================================
  Files           112      112              
  Lines         43286    43344      +58     
  Branches       9572     9576       +4     
============================================
+ Hits          14789    14820      +31     
- Misses        25478    25511      +33     
+ Partials       3019     3013       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

exit 1
fi

STAGED_REPO_ID=$(echo $REPO_REQUEST_RESPONSE | xmllint -xpath "//stagedRepositoryId/text()" )
Copy link
Member

Choose a reason for hiding this comment

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

This is failing for me with:

Usage : xmllint [options] XMLfiles ...

I am using this version:

% xmllint --version
xmllint: using libxml version 20913
   compiled with: Threads Tree Output Push Reader Patterns Writer SAXv1 FTP HTTP DTDValid HTML Legacy C14N Catalog XPath XPointer XInclude ICU ISO8859X Unicode Regexps Automata Schemas Schematron Modules Debug Zlib 

exit 1
fi

STAGED_REPO_ID=$(echo $REPO_REQUEST_RESPONSE | xmllint -xpath "//stagedRepositoryId/text()" )
Copy link
Member

Choose a reason for hiding this comment

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

This fixes it for me.

The - at the end indicates stdin input

Suggested change
STAGED_REPO_ID=$(echo $REPO_REQUEST_RESPONSE | xmllint -xpath "//stagedRepositoryId/text()" )
STAGED_REPO_ID=$(echo $REPO_REQUEST_RESPONSE | xmllint --xpath "//stagedRepositoryId/text()" -)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this. I tested parts of this script independently and once I put it all together I was not able to test everything since it would fail before this stage!

Comment on lines 99 to 110
# check permission
PERMITTED_REQUEST="-u $ASF_USERNAME:$ASF_PASSWORD \
-H "Content-Type:application/xml" \
$NEXUS_ROOT/profiles/$NEXUS_PROFILE/start"
PERMITTED=$(curl -s -o /dev/null -w "%{http_code}" $PERMITTED_REQUEST)

if [ "$PERMITTED" != "200" ]
then
echo "Nexus replied with a status code: $PERMITTED"
echo "You may not be authorized to perform this action"
exit 1
fi
Copy link
Member

Choose a reason for hiding this comment

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

This permission check does not work for me. I get a 405 response. I just commented this section out and the rest of the script worked fine, other than one small edit I had to make (see separate comment for this).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check is probably incorrect. It worked for me because I did not have the permission and this correctly caught that. But when you do have permission, it reports an error that basically says you can't do this.
Removing it.

Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

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

LGTM! This is working for me now. Thanks @parthchandra

@andygrove andygrove merged commit c3023c5 into apache:main Sep 21, 2024
74 checks passed
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.

3 participants