Skip to content

feat(okta): Deprecation notice for OKTA_API_TOKEN env variable support #10887

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.

Sign up for GitHub

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

Jump to bottom
Merged
kodiakhq merged 3 commits into from
May 24, 2023
Merged

feat(okta): Deprecation notice for OKTA_API_TOKEN env variable support #10887

kodiakhq merged 3 commits into from
May 24, 2023

Conversation

@candiduslynx
Copy link
Contributor

@candiduslynx candiduslynx commented May 22, 2023

No description provided.

@candiduslynx candiduslynx requested review from a team and bbernays and removed request for a team May 22, 2023 14:14
bbernays
bbernays requested changes May 22, 2023
View reviewed changes
Copy link
Collaborator

@bbernays bbernays left a comment

Choose a reason for hiding this comment

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

Why would we want to break backwards compatibility for this?

@candiduslynx
Copy link
Contributor Author

candiduslynx commented May 22, 2023

Why would we want to break backwards compatibility for this?

@bbernays

It's a cleanup as we did in Git(Lab?Hub?) and Tailscale.

Additionally, I'd like to merge this along with the v3 migration, as that one would already be a breaking change.

@bbernays
Copy link
Collaborator

bbernays commented May 22, 2023

Why would we want to break backwards compatibility for this?

@bbernays

It's a cleanup as we did in Git(Lab?Hub?) and Tailscale.

Additionally, I'd like to merge this along with the v3 migration, as that one would already be a breaking change.

What is the improved experience or functionality that users will get by us forcing them to adjust their configurations?

But the V3 migration shouldn't require users to adjust their configurations other than the destination version...

@candiduslynx
Copy link
Contributor Author

candiduslynx commented May 22, 2023

What is the improved experience or functionality that users will get by us forcing them to adjust their configurations?

We streamline the code base here (no direct env lookups in plugins). It's more of the dev experience change.

But the V3 migration shouldn't require users to adjust their configurations other than the destination version...

It will still be a breaking change per the types changes and the need to use updated destinations as well.
So, it feels like a good moment to add an additional breaking change.

hermanschaaf
hermanschaaf previously requested changes May 22, 2023
View reviewed changes
Copy link
Member

@hermanschaaf hermanschaaf left a comment

Choose a reason for hiding this comment

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

I agree, let's not do a breaking change only for the sake of removing a few lines / consistency with other plugins.

Instead, maybe we can start by logging a warning if you use OKTA_API_TOKEN without setting token in the config, and say this is marked for deprecation and will be removed in a future version?

@candiduslynx candiduslynx force-pushed the feat/okta/drop-explicit-env branch 2 times, most recently from aed1193 to 564691b Compare May 23, 2023 11:09
@cq-bot cq-bot removed the okta label May 23, 2023
@candiduslynx candiduslynx changed the title feat(okta)!: Make token param required & drop OKTA_API_TOKEN env variable support feat(okta): Deprecation notice for OKTA_API_TOKEN env variable support May 23, 2023
@candiduslynx candiduslynx force-pushed the feat/okta/drop-explicit-env branch from 564691b to 04e8998 Compare May 23, 2023 11:12
@cq-bot cq-bot added the okta label May 23, 2023
@candiduslynx candiduslynx added automerge Automatically merge once required checks pass and removed no automerge labels May 23, 2023
@candiduslynx
Copy link
Contributor Author

candiduslynx commented May 23, 2023

@hermanschaaf @bbernays
I've changed the PR to only print out a warning about this variable + added the warning to the config example & overview.

bbernays
bbernays reviewed May 23, 2023
View reviewed changes
@candiduslynx candiduslynx requested a review from bbernays May 23, 2023 14:38
@kodiakhq kodiakhq bot merged commit 528daaa into main May 24, 2023
@kodiakhq kodiakhq bot deleted the feat/okta/drop-explicit-env branch May 24, 2023 07:06
@cq-bot cq-bot mentioned this pull request May 24, 2023
Merged
kodiakhq bot pushed a commit that referenced this pull request May 30, 2023
@cq-bot
chore(main): Release plugins-source-okta v3.0.0 (#10905)
d26981a 
🤖 I have created a release *beep* *boop*
---


## [3.0.0](plugins-source-okta-v2.4.0...plugins-source-okta-v3.0.0) (2023-05-30)


### ⚠ BREAKING CHANGES

* This release introduces an internal change to our type system to use [Apache Arrow](https://arrow.apache.org/). This should not have any visible breaking changes, however due to the size of the change we are introducing it under a major version bump to communicate that it might have some bugs that we weren't able to catch during our internal tests. If you encounter an issue during the upgrade, please submit a [bug report](https://github.com/cloudquery/cloudquery/issues/new/choose). You will also need to update destinations depending on which one you use:
    - Azure Blob Storage >= v3.2.0
    - BigQuery >= v3.0.0
    - ClickHouse >= v3.1.1
    - DuckDB >= v1.1.6
    - Elasticsearch >= v2.0.0
    - File >= v3.2.0
    - Firehose >= v2.0.2
    - GCS >= v3.2.0
    - Gremlin >= v2.1.10
    - Kafka >= v3.0.1
    - Meilisearch >= v2.0.1
    - Microsoft SQL Server >= v4.2.0
    - MongoDB >= v2.0.1
    - MySQL >= v2.0.2
    - Neo4j >= v3.0.0
    - PostgreSQL >= v4.2.0
    - S3 >= v4.4.0
    - Snowflake >= v2.1.1
    - SQLite >= v2.2.0

### Features

* **okta:** Deprecation notice for `OKTA_API_TOKEN` env variable support ([#10887](#10887)) ([528daaa](528daaa))
* Update to use [Apache Arrow](https://arrow.apache.org/) type system ([#10983](#10983)) ([03473dd](03473dd))


### Bug Fixes

* **deps:** Update module github.com/cloudquery/plugin-sdk/v3 to v3.6.7 ([#11043](#11043)) ([3c6d885](3c6d885))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bbernays bbernays bbernays approved these changes

@hermanschaaf hermanschaaf Awaiting requested review from hermanschaaf

Assignees

No one assigned

Labels

automerge Automatically merge once required checks pass

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@candiduslynx @bbernays @hermanschaaf @cq-bot