Extract the mock api url from code to pipeline env #2388
Conversation
|
@Yuki-YuXin I'd like to see this PR in action, i.e. run the tests using the new env var. Could you take a look at that? |
|
@Yuki-YuXin there are more instances of the mock API URL in the code base, that haven't been replaced as part of this PR. Some of them might be in MSAL. Could you take a look at that? |
Yuki-YuXin
added
No-4j-Changelog
Yuki-YuXin
added
Skip-Consumers-Check
|
Summarize the discussion with Silviu in terms of this work item: IOS don’t run the tests in the yml file. They run them locally, have a stash that we apply and then discard. As in they don’t run as part of the pipeline. They have different suite for unit which run in the pipeline and integration with mock api which run only locally. |
This reverts commit 5afb03a.
What's your point here @Yuki-YuXin ? |
| @@ -30,8 +30,8 @@ import java.net.URL | |||
| */ | |||
| interface ApiConstants { | |||
| companion object { | |||
| const val BASEPATH = "https://native-auth-mock-api.azurewebsites.net/" | |||
| private const val BASE_REQUEST_PATH = BASEPATH + "1234/" | |||
| val BASEPATH: String? by lazy { System.getenv("MOCK_API_URL") } | |||
There was a problem hiding this comment.
This is not how we should be dealing with env vars. They should be set as part of the gradle script, in BuildConfig vars. E.g. look at how the Labs API secret is set in various build.gradle files: buildConfigField("String", "LAB_CLIENT_SECRET", "\"$labClientSecret\"").
There was a problem hiding this comment.
Let me know if you need some help with this.
| @@ -51,7 +50,7 @@ class NativeAuthOAuth2Configuration( | |||
|
|
|||
| companion object { | |||
| //Base url for the mock API to make Native Auth calls. See the swagger at | |||
| // https://native-auth-mock-api.azurewebsites.net/doc#/ for all possible urls | |||
| // $(MOCK_API_URL)/doc#/ for all possible urls | |||
| private const val MOCK_API_URL_WITH_NATIVE_AUTH_TENANT = "https://native-auth-mock-api.azurewebsites.net/lumonconvergedps.onmicrosoft.com" | |||
There was a problem hiding this comment.
Mock API value is still set here. Can you please do a ctrl-F in the entire project for this API value string and remove all occurrences?
There was a problem hiding this comment.
I want to first let the PR passed first using the concrete string since it's not a test file.
| @@ -17,6 +21,8 @@ pool: | |||
| jobs: | |||
| - job: build_test | |||
| displayName: Build & Test | |||
| env: | |||
| MOCK_API_URL: https://native-auth-mock-api.azurewebsites.net/ | |||
There was a problem hiding this comment.
Why is this here? Shouldn't this PR be introducing the integration with the environment var, so we can test that?
They don't seem to intend to integrate the mock api into the pipeline in the center server and they can easily accept the changes of mock api because every time they use the url locally. |
From the experience of cert PR, if we can pass the PR pipeline verification check, we prove that the new env var actually work |
Summary:
Please go to the PR for further workhttps://github.com//pull/2387
This PR is closed because of the messy state during test.