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

feat: fetch user credentials in sdk #78

Merged
merged 8 commits into from
Jan 8, 2025

Conversation

mariatsji
Copy link
Contributor

No description provided.

@mariatsji mariatsji requested review from tenstad and erikgb January 3, 2025 10:05
@mariatsji mariatsji self-assigned this Jan 3, 2025
default:
return nil, fmt.Errorf("error: list credentials unexpected status code: %d", resp.StatusCode)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

No test of this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None of the cloudian invoking endpoints have tests. We would have to introduce some interfaces and start mocking or stubbing I guess.
I've only tested curl against the cloudian API docs

Copy link
Member

Choose a reason for hiding this comment

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

We should probably add some kind of Go REST module that we can easily stub out. Untested code is no good.

Copy link
Member

@erikgb erikgb left a comment

Choose a reason for hiding this comment

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

LGTM, but I don't like that a lot of the code is untested.

)

func TestSecretUnmarshal(t *testing.T) {
jsonString := `[{"accessKey":"124","secretKey":"x+2","createDate":1735894172440,"active":true}]`
Copy link
Member

Choose a reason for hiding this comment

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

Don't think we need to test the builtin go marshalling. Suggesting to simply remove the test.

Obfuscating also seems a bit overkill, as the thing running this has access, and we should not print the thing in production.

Copy link
Member

@tenstad tenstad left a comment

Choose a reason for hiding this comment

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

The sdk func LGTM! Suggesting to merge just that, and remove secret.go and secret_test.go and focus on the important parts now. Add tests when they provide real value.

mariatsji and others added 2 commits January 8, 2025 07:23
@mariatsji
Copy link
Contributor Author

Merging as-is with both Secret and json roundtrip - these can be adjusted with new PRs

@mariatsji mariatsji merged commit ad0ef0a into statnett:main Jan 8, 2025
6 checks passed
@mariatsji mariatsji deleted the fetch-credentials branch January 8, 2025 08:23
@statnett-bot statnett-bot bot mentioned this pull request Jan 8, 2025
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.

3 participants