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.

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

♻️ (gocardless) renamed nordigen_* secrets to gocardless_* #293

Merged
merged 4 commits into from
Jan 9, 2024
Merged

♻️ (gocardless) renamed nordigen_* secrets to gocardless_* #293

merged 4 commits into from
Jan 9, 2024

Conversation

MatissJanis
Copy link
Member

@MatissJanis MatissJanis commented Dec 15, 2023 •
edited
Loading

Final change for: actualbudget/actual#1244

@MatissJanis MatissJanis changed the base branch from matiss/docs to master December 15, 2023 19:23
@MatissJanis MatissJanis mentioned this pull request Dec 15, 2023
Closed
@MatissJanis MatissJanis marked this pull request as ready for review December 15, 2023 19:30
@twk3 twk3 self-requested a review December 31, 2023 22:24
@twk3
Copy link
Contributor

twk3 commented Dec 31, 2023

@MatissJanis is there a matching MR for updating the secret-set in desktop-client? It's still pointing at nordigen there. Currently with this change you can no longer init gocardless fresh.

Might be easiest to leave both sets of constants here in SecretName, both pointing to the new names. Then following up with a final MR to remove the extra constants after the update in desktop-client is released.

@twk3 twk3 removed their request for review December 31, 2023 22:30
@MatissJanis MatissJanis mentioned this pull request Jan 6, 2024
Merged
@MatissJanis
Copy link
Member Author

Good catch @twk3 !

We can update both of these variables in-sync. Here's the web PR: actualbudget/actual#2181

@trafico-bot trafico-bot bot added ✅ Approved Pull Request has been approved and can be merged and removed 🔍 Ready for Review labels Jan 7, 2024
@MatissJanis MatissJanis requested a review from twk3 January 8, 2024 17:43
twk3
twk3 approved these changes Jan 9, 2024
View reviewed changes
Copy link
Contributor

@twk3 twk3 left a comment

Choose a reason for hiding this comment

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

Looks good! Ship it!.

I tested with the linked actual-web PR, and confirmed the migrations worked for secrets setup before the change, and that fresh setups also work after the change.

Also did a quick sanity that gocardless bank setup was still working using the sandbox institution https://developer.gocardless.com/bank-account-data/sandbox

@MatissJanis MatissJanis merged commit ff073b1 into master Jan 9, 2024
8 checks passed
@MatissJanis MatissJanis deleted the matiss/gocardless-rename branch January 9, 2024 19:21
@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved Pull Request has been approved and can be merged labels Jan 9, 2024
@twk3 twk3 mentioned this pull request Feb 13, 2024
Merged
MMichotte pushed a commit to MMichotte/actual-server that referenced this pull request Sep 9, 2024
@MatissJanis @MMichotte
♻️ (gocardless) renamed nordigen_* secrets to gocardless_* (actualbud…
51f00b2 
…get#293)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youngcw youngcw youngcw approved these changes

@twk3 twk3 twk3 approved these changes

Assignees
No one assigned
Labels
✨ Merged Pull Request has been merged successfully
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MatissJanis @twk3 @youngcw