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

ci_credentials: fix overwriting 'data' before getting nextPageToken #24265

Merged
merged 2 commits into from
Mar 21, 2023

Conversation

erohmensing
Copy link
Contributor

@erohmensing erohmensing commented Mar 21, 2023

What

  • Fix issue where VERSION=dev ci_credentials all write-to-storage would only write 100 secrets instead of the full 408
  • Updates to readme

How

Use diffferent variable names when getting data from various endpoints. Previously we were overwriting the data variable with different contents - therefore the nextPageToken was either not there or invalid by the time we tried to use it, so we never got the next page.

Cherrypicked from #24254 as it was quasi-necessary to run all CATs locally (you can run ci_credentials per connector, but that's slow), and I figured other people might be able to use this now.

@erohmensing erohmensing requested a review from a team March 21, 2023 02:16
Copy link
Contributor

@bnchrch bnchrch left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -44,12 +44,19 @@ The `VERSION=dev` will make it so it knows to use your local current working dir
ci_credentials --help
```

### Write to storage
### Write credentials for a specific connector to local storage
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

To download GSM secrets to `airbyte-integrations/connectors/source-bings-ads/secrets`:
```bash
ci_credentials source-bing-ads write-to-storage
```

### Write credentials for all connectors to local storage
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

@@ -81,8 +81,8 @@ def __load_gsm_secrets(self) -> List[RemoteSecret]:
if next_token:
params["pageToken"] = next_token

data = self.api.get(url, params=params)
for secret_info in data.get("secrets") or []:
all_secrets_data = self.api.get(url, params=params)
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 Nice rename

@@ -103,14 +103,14 @@ def __load_gsm_secrets(self) -> List[RemoteSecret]:
self.logger.info(f"found GSM secret: {log_name} = > {filename}")

versions_url = f"https://secretmanager.googleapis.com/v1/{secret_name}/versions"
data = self.api.get(versions_url)
enabled_versions = [version["name"] for version in data["versions"] if version["state"] == "ENABLED"]
versions_data = self.api.get(versions_url)
Copy link
Contributor

Choose a reason for hiding this comment

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

(Dreaming) If only python had an immutable variable feature

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lol, too spoiled by java's pickiness!

@erohmensing erohmensing merged commit 8f65d61 into master Mar 21, 2023
@erohmensing erohmensing deleted the ella/bugfix-ci-creds branch March 21, 2023 19:45
erohmensing added a commit that referenced this pull request Mar 22, 2023
…24265)

* ci_credentials: fix overwriting 'data' before getting nextPageToken

* Updates to readme
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants