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

Add guidelines for code contributions #3976

Merged
merged 3 commits into from
Jul 23, 2022
Merged

Add guidelines for code contributions #3976

merged 3 commits into from
Jul 23, 2022

Conversation

adnapibar
Copy link
Contributor

Signed-off-by: Rabi Panda [email protected]

Description

A set of guidelines for contributors to help them deciding whether a feature should be included in OpenSearch.

Issues Resolved

N/A

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.

A set of guidelines to decide whether a feature should be included in OpenSearch.

Signed-off-by: Rabi Panda <[email protected]>
@adnapibar adnapibar requested review from a team and reta as code owners July 21, 2022 21:55
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Co-authored-by: Marc Handalian <[email protected]>

Signed-off-by: Rabi Panda <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Copy link

@elfisher elfisher left a comment

Choose a reason for hiding this comment

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

This lgtm. I've left some minor wording suggestions.


If you believe that your feature is going to fulfill a need for most users of OpenSearch, then this belongs in OpenSearch. However, we don't want every feature to be built into the server, so if the feature requires additional permissions or brings in extra dependencies it should instead be included as a core module.

**Is your feature a common dependency across multiple plugins?**

Choose a reason for hiding this comment

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

I like this a lot. I think this also is a good criteria for being an extension point too.

CONTRIBUTING.md Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

Codecov Report

Merging #3976 (33aabca) into main (b08a2b8) will increase coverage by 0.06%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #3976      +/-   ##
============================================
+ Coverage     70.56%   70.62%   +0.06%     
- Complexity    56780    56797      +17     
============================================
  Files          4573     4573              
  Lines        273323   273323              
  Branches      40086    40086              
============================================
+ Hits         192859   193028     +169     
+ Misses        64239    64019     -220     
- Partials      16225    16276      +51     
Impacted Files Coverage Δ
.../java/org/opensearch/node/NodeClosedException.java 50.00% <0.00%> (-50.00%) ⬇️
...h/action/ingest/SimulateDocumentVerboseResult.java 60.71% <0.00%> (-39.29%) ⬇️
...search/aggregations/pipeline/HoltWintersModel.java 21.47% <0.00%> (-32.22%) ⬇️
...min/cluster/snapshots/get/GetSnapshotsRequest.java 52.63% <0.00%> (-31.58%) ⬇️
...aggregations/bucket/terms/heuristic/ChiSquare.java 44.82% <0.00%> (-27.59%) ⬇️
...va/org/opensearch/client/sniff/SnifferBuilder.java 72.72% <0.00%> (-27.28%) ⬇️
...opensearch/snapshots/SnapshotRestoreException.java 25.00% <0.00%> (-25.00%) ⬇️
...rch/indices/recovery/RetryableTransportClient.java 56.81% <0.00%> (-25.00%) ⬇️
...ations/bucket/terms/heuristic/ScriptHeuristic.java 5.55% <0.00%> (-20.38%) ⬇️
...ain/java/org/opensearch/geometry/MultiPolygon.java 80.00% <0.00%> (-20.00%) ⬇️
... and 454 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

I like it. I would also say something about whether the feature can have alternate implementations. For example, for job-scheduler we ask whether job-scheduler be included in core -> will we ever want two different implementations of job-scheduler. If the answer were a definite yes, we'd not want it in core.

@nknize
Copy link
Collaborator

nknize commented Jul 22, 2022

...will we ever want two different implementations of job-scheduler. If the answer were a definite yes, we'd not want it in core.

I don't use this criteria. Snapshot repository modules, for example, have multiple implementations in core (S3, Azure, GCS). They're in the core as modules because we want them installed as part of the min default experience. I could see two different job schedulers running in parallel using different implementations (cron vs task).

@adnapibar adnapibar merged commit efde8c5 into opensearch-project:main Jul 23, 2022
@navneet1v
Copy link
Contributor

Does this change encourages that as the feature owner we should create plugins or move to a module within the OpenSearch repo? This is bit unclear here. what is the thought process for the same?

@CEHENKLE
Copy link
Member

We want things to get built in the place makes the most sense for them to be built :)

From my POV, it's less about encouraging features owners to build in one place or the other, and more about making sure they're thinking about what is the best place to build things using a lens of commonality.

Does that makes sense @navneet1v ?

@navneet1v
Copy link
Contributor

We want things to get built in the place makes the most sense for them to be built :)

From my POV, it's less about encouraging features owners to build in one place or the other, and more about making sure they're thinking about what is the best place to build things using a lens of commonality.

Does that makes sense @navneet1v ?

Yes it make sense. Thanks for providing the clarity.

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.

9 participants