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

Fix Javadoc errors in module client/rest #685

Merged

Conversation

gzurowski
Copy link
Contributor

Description

Fix Javadoc comments to fix Javadoc errors as described in issue #221.

Issues Resolved

Fixes Javadoc errors in the client/rest module.

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 9bbb5c9

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 9bbb5c9

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Precommit failure 9bbb5c9
Log 389

@adnapibar adnapibar self-requested a review May 11, 2021 18:19
Copy link
Contributor

@adnapibar adnapibar left a comment

Choose a reason for hiding this comment

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

@gzurowski Thanks for the contribution. I have approved it with a few minor comments :)

gzurowski added 2 commits May 12, 2021 09:14
Signed-off-by: Gregor Zurowski <[email protected]>
@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 514433e

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 514433e

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Precommit failure 514433e
Log 394

@gzurowski
Copy link
Contributor Author

@adnapibar Thanks for your review, I've made the suggested updates.

Copy link
Contributor

@setiah setiah 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 great @gzurowski . It fixes all 64 errors due to missing javadocs in client:rest module, which is pretty awesome! - https://gist.github.com/setiah/8c6805822a790bd3536a23ea75d25b29

PS: Validatied by running ./gradlew client:rest:missingJavadoc

@setiah setiah merged commit e7692cb into opensearch-project:javadoc May 12, 2021
@dblock
Copy link
Member

dblock commented May 12, 2021

Noticed it was merged into the javadoc branch. Was that intentional? This needs to go into main, doesn't it? What are the labels we need for this @adnapibar?

@setiah
Copy link
Contributor

setiah commented May 12, 2021

Yes this was intentional but I agree this needs to go in main as well. The problem is, missingJavadoc check is only available on javadoc branch. It is not added to main because of issue #449 . Once that is fixed, we can merge javadoc branch to main (while keeping missingJavadoc checks excluded from gradlew check) which will also merge all incoming javadocs. I think we should prioritize the fix as it gets harder to maintain a feature branch. However, until that happens, we have below options

  1. Merge and keep new incoming javadocs in javadoc branch.Fix Fix precommit check failures with missingJavadoc task #449 asap, and then rebase and merge javadoc branch commits into main.
  2. Merge new incoming javadocs in javadoc branch and port it to main branch as well. (But then it becomes a backporting nightmare as there are other branches as well)
  3. Merge incoming javadocs to main branch (and backport to 1.x as other PRs) and let javadoc branch pull from main. However the problem with merging to main is there is no check available to validate the new javadocs. The checks are only available in javadoc branch. So if we go with this, contributor has to first verify against javadoc branch, then push to main and 1.x branch. (javadoc branch can be maintained by maintainers)
    @dblock i'm fine with both 1 & 3, but 3 seems to have slightly more friction. WDYT?

@nknize
Copy link
Collaborator

nknize commented May 13, 2021

This is the second time I've seen this precommit failure:

* What went wrong:
A problem was found with the configuration of task ':client:benchmark:validateNebulaPom' (type 'PomValidationTask').
> File '/var/CITOOL/workflow/OpenSearch_CI/PR_Checks/Gradle_Precommit/search/client/benchmark/build/publications/nebula/pom-default.xml' specified for property 'pomFile' does not exist.

We shouldn't get into a habit of merging w/ failing precommit. Is this a CI config issue? Red herring?

@setiah
Copy link
Contributor

setiah commented May 14, 2021

This is the second time I've seen this precommit failure:

* What went wrong:
A problem was found with the configuration of task ':client:benchmark:validateNebulaPom' (type 'PomValidationTask').
> File '/var/CITOOL/workflow/OpenSearch_CI/PR_Checks/Gradle_Precommit/search/client/benchmark/build/publications/nebula/pom-default.xml' specified for property 'pomFile' does not exist.

We shouldn't get into a habit of merging w/ failing precommit. Is this a CI config issue? Red herring?

@nknize the precommit fails because of #449 . That's why we are keeping these changes in separate feature (javadoc) branch, until we fix that. Good news though, I have figured a fix for #449 , and once that's merged, the precommit should start passing and then we can merge javadoc branch to main

setiah added a commit that referenced this pull request May 18, 2021
* Adds a gradle plugin to validate missing javadocs

Use `./gradlew missingJavadoc` to validate missing javadocs.
Currently this task fails because several modules are missing
appropriate javadocs. Once added, this should pass.
Also, precommit PomValidation check currently fails with missing Javadoc
plugin, that needs to be fixed -
#449
Thus keeping this in a separate feature branch.

Signed-off-by: Himanshu Setia <[email protected]>

* Fix Javadoc errors in module `client/rest` (#685)

* Fix Javadoc errors in client/rest module

Signed-off-by: Gregor Zurowski <[email protected]>

* Add package info file in client/rest module

Signed-off-by: Gregor Zurowski <[email protected]>

* Fix typos

Signed-off-by: Gregor Zurowski <[email protected]>

* Add exception documentation to Javadoc

Signed-off-by: Gregor Zurowski <[email protected]>

* Fixes precommit task configuration failures due to newly added missin… (#707)

* Fixes precommit task configuration failures due to newly added missingJavadoc task

Signed-off-by: Himanshu Setia <[email protected]>

* Fixes javadoc task errors due to PR#685

Signed-off-by: Himanshu Setia <[email protected]>

* Updated CONTRIBUTING.md for info on javadocs

Signed-off-by: Himanshu Setia <[email protected]>

* Correcting licenses and naming

Signed-off-by: Himanshu Setia <[email protected]>

* Correcting version info

Signed-off-by: Himanshu Setia <[email protected]>

Co-authored-by: Gregor Zurowski <[email protected]>
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.

6 participants