Skip to content

Conversation

@PSanetra
Copy link
Contributor

@PSanetra PSanetra commented Oct 9, 2024

Closes #81

Changes

  • Make previously required parameters of oauth_channel.create_oauth2_client_credentials_channel() and oauth_channel.create_camunda_cloud_channel() optional

API Updates

New Features (required)

oauth_channel.create_oauth2_client_credentials_channel() and oauth_channel.create_camunda_cloud_channel() support the following environment variables:

  • ZEEBE_ADDRESS
  • CAMUNDA_CLUSTER_ID
  • CAMUNDA_CLUSTER_REGION
  • CAMUNDA_CLIENT_ID
  • ZEEBE_CLIENT_ID
  • CAMUNDA_CLIENT_SECRET
  • ZEEBE_CLIENT_SECRET
  • CAMUNDA_OAUTH_URL
  • ZEEBE_AUTHORIZATION_SERVER_URL
  • CAMUNDA_CREDENTIALS_SCOPES
  • CAMUNDA_TOKEN_AUDIENCE
  • ZEEBE_TOKEN_AUDIENCE

Checklist

  • Unit tests
  • Documentation

@CLAassistant
Copy link

CLAassistant commented Oct 9, 2024

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Oct 9, 2024

Pull Request Test Coverage Report for Build 11739142105

Details

  • 74 of 74 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 97.117%

Totals Coverage Status
Change from base Build 11659466005: 0.2%
Covered Lines: 1078
Relevant Lines: 1110

💛 - Coveralls

@PSanetra PSanetra force-pushed the add-environment-variables branch 3 times, most recently from d2d7ac9 to a2f3e0a Compare October 9, 2024 14:29
@felicijus
Copy link
Contributor

felicijus commented Oct 9, 2024

@dimastbk I would like to check the Camunda Cloud implementation side of this again, review should be in tomorrow.

@PSanetra thank you, you were quicker I also had this planned as a pull-request.

@felicijus
Copy link
Contributor

Let's wait for #514 and #510 to be closed.

Copy link
Contributor

@felicijus felicijus left a comment

Choose a reason for hiding this comment

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

Looks promising. 🚀

PSanetra added a commit to PSanetra/pyzeebe that referenced this pull request Oct 11, 2024
PSanetra added a commit to PSanetra/pyzeebe that referenced this pull request Oct 12, 2024
@PSanetra PSanetra force-pushed the add-environment-variables branch from 2dc98ec to 0ddbdac Compare October 12, 2024 15:07
@felicijus
Copy link
Contributor

I will review again 🚀

PSanetra added a commit to PSanetra/pyzeebe that referenced this pull request Oct 12, 2024
@PSanetra
Copy link
Contributor Author

@felicijus @dimastbk do you have time for another review?

@felicijus
Copy link
Contributor

felicijus commented Oct 18, 2024

Sorry forgot to finish mine... Released it now. but thats not everything.

Copy link
Contributor

@felicijus felicijus left a comment

Choose a reason for hiding this comment

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

This should cover everything for now.

If we use a sentinal like this:

Unset = str("UNSET")

We solve the problem of None as a valid value and pseudo Optional arguments, meaning we can eliminate them.
This means for real Optional arguments the User can define None as a valid value. 🤖

PSanetra added a commit to PSanetra/pyzeebe that referenced this pull request Oct 21, 2024
BREAKING CHANGES: Default values for arguments of create_oauth2_client_credentials_channel() and create_camunda_cloud_channel() have changed to Unset. Also order of create_camunda_cloud_channel() arguments was changed.
@PSanetra PSanetra force-pushed the add-environment-variables branch from 12535fc to 4229505 Compare October 21, 2024 10:09
@PSanetra
Copy link
Contributor Author

@felicijus I have applied your latest suggestions, but needed to make some adjustments. E.g. the functions get_zeebe_address, get_camunda_oauth_url, get_camunda_cluster_region and get_camunda_token_audience need to have an optional default_value argument, which need to be considered as fallback in the case of usages in create_camunda_cloud_channel,

Also I have seen that version 4.0.0 was released, so I think the change of argument order in create_camunda_cloud_channel and probably also the introduction of the new Unset default values need to be considered as breaking changes.

@felicijus
Copy link
Contributor

felicijus commented Oct 21, 2024

Thank you. I will look into it this afternoon.
But i think handing over the default value is quite nice ✅.

@felicijus
Copy link
Contributor

Docs will be done by you @PSanetra ?

@PSanetra
Copy link
Contributor Author

@felicijus I can do that, but I am not sure where to put them. Do you have a suggestion? I think it should generally be just documentation of the supported environment variables, right?

@felicijus
Copy link
Contributor

Yes general support with what is supported as a seperate section, and add a short description to each channel creating function and the defaults used.

About the breaking changes, yes and no... At least for the oauth channel creation - no. There was no support for Environment variables at all and you also had no imposter None as default. ✅
For create_secure_channel and create_insecure_channel it can be considered a breaking change. ⚠️

So I guess 4.1.0 or whatever @dimastbk decides.

Copy link
Contributor

@felicijus felicijus left a comment

Choose a reason for hiding this comment

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

✅ Maybe try to reach higher test coverage.

PSanetra added a commit to PSanetra/pyzeebe that referenced this pull request Oct 29, 2024
BREAKING CHANGES: Default values for arguments of create_oauth2_client_credentials_channel() and create_camunda_cloud_channel() have changed to Unset. Also order of create_camunda_cloud_channel() arguments was changed.
@PSanetra PSanetra force-pushed the add-environment-variables branch from 529d0b7 to 5b9648c Compare October 29, 2024 10:55
@PSanetra
Copy link
Contributor Author

@felicijus I have added some documentation can you review it? I did not write reStructuredText docs before.

I have also added some tests which should improve coverage.

Copy link
Contributor

@felicijus felicijus left a comment

Choose a reason for hiding this comment

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

I suggested some changes...

Please add this to the end of docs/channels_reference.rst to complete Channel Reference.

Utilities (Environment) 
-----------------------

.. autofunction:: pyzeebe.channel.utils.get_zeebe_address

.. autofunction:: pyzeebe.channel.utils.get_camunda_oauth_url

.. autofunction:: pyzeebe.channel.utils.get_camunda_client_id

.. autofunction:: pyzeebe.channel.utils.get_camunda_client_secret

.. autofunction:: pyzeebe.channel.utils.get_camunda_cluster_id

.. autofunction:: pyzeebe.channel.utils.get_camunda_cluster_region

.. autofunction:: pyzeebe.channel.utils.get_camunda_token_audience

.. autofunction:: pyzeebe.channel.utils.get_camunda_address

@PSanetra PSanetra force-pushed the add-environment-variables branch from 422543d to ae19fdd Compare October 31, 2024 12:41
@PSanetra
Copy link
Contributor Author

@felicijus Alright I have applied the suggestions

Copy link
Contributor

@felicijus felicijus left a comment

Choose a reason for hiding this comment

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

Missed some rst formatting.

Inline code:

``"localhost:26500"``

After that we are done I think. ✅

@PSanetra
Copy link
Contributor Author

@felicijus Alright I have applied that suggestion as well. I think when you want to merge this PR it makes sense to squash all the commit into one.

Copy link
Contributor

@felicijus felicijus left a comment

Choose a reason for hiding this comment

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

Nice one. ✅

@dimastbk Please have a look and see if this can be released in 4.0.x...

PSanetra and others added 8 commits November 7, 2024 09:49
BREAKING CHANGES: Default values for arguments of create_oauth2_client_credentials_channel() and create_camunda_cloud_channel() have changed to Unset. Also order of create_camunda_cloud_channel() arguments was changed.
Co-authored-by: Felix Schneider <[email protected]>
@PSanetra PSanetra force-pushed the add-environment-variables branch from f647ecd to f951a9a Compare November 7, 2024 08:58
@PSanetra
Copy link
Contributor Author

PSanetra commented Nov 7, 2024

@dimastbk I have rebased the PR onto master. I have also fixed the linting issues, found in the CI, but I am not sure were you found the errors, regarding the indentation in the docs.

@PSanetra
Copy link
Contributor Author

PSanetra commented Nov 8, 2024

@dimastbk I have resolved the remaining indentation issues.

@dimastbk dimastbk merged commit 0eed3dc into camunda-community-hub:master Nov 8, 2024
20 checks passed
@dimastbk
Copy link
Collaborator

dimastbk commented Nov 8, 2024

Thanks!

And @felicijus thanks for review

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.

Use environment variables to configure Zeebe

5 participants