Skip to content

Remove hardcoded en-us locale from docs site links.#904

Merged
zikalino merged 3 commits intoAzure:masterfrom
sptramer:fix-locale-links
Sep 19, 2019
Merged

Remove hardcoded en-us locale from docs site links.#904
zikalino merged 3 commits intoAzure:masterfrom
sptramer:fix-locale-links

Conversation

@sptramer
Copy link
Copy Markdown
Contributor

No description provided.

@azuresdkci
Copy link
Copy Markdown

If this PR is for a new extension or change to an existing extension, use the following to try out the changes in this PR:

docker run -it microsoft/azure-cli:latest
export EXT=<NAME>
pip install --upgrade --target ~/.azure/cliextensions/$EXT "git+https://github.com/sptramer/azure-cli-extensions.git@fix-locale-links#subdirectory=src/$EXT&egg=$EXT"

@limingu limingu removed their request for review August 15, 2019 18:06
@mmyyrroonn
Copy link
Copy Markdown
Contributor

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suggest we back out changes under folders of verndored_sdks as those are generated files from swagger specs, and the next round of code-gen will wipe out the change from this PR. If we like to address the root cause, we should update specs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@yugangw-msft If everything is generated from the Swagger specs, then this PR should be closed and we can address the root problem (which is the Swagger.) Otherwise feel free to merge it - I've taken an item to look into hardcoded en-us in Swagger.

@yugangw-msft
Copy link
Copy Markdown
Contributor

https://azurecliext.blob.core.windows.net/release/azure_cli_ml-1.0.55-py2.py3-none-any.whl

@myronfanqiu, this extension's source code us hosted at external repository. CC: @gogowings

@sptramer
Copy link
Copy Markdown
Contributor Author

sptramer commented Aug 30, 2019

@yugangw-msft I noticed this hadn't been merged and looked into the error happening on CI. I found that it's actually because the test is expecting en-us to be in the link:

AssertionError:
  {'aze[553 chars].com/en-us/azure/machine-learning/service/'}}}[526 chars].57'} !=
  {'aze[553 chars].com/azure/machine-learning/service/'}}}, 'ext[520 chars].57'}

It looks like this is because it's comparing the non-en-us link against the currently published information, which is en-us. That means it looks like this test doesn't take into account the possibility that reference links were incorrect and need to be fixed.

I'd like to request that this PR be merged as a result, since I'm not sure of a good way to fix this test.

@yugangw-msft
Copy link
Copy Markdown
Contributor

CC @zikalino owner of CLI extension repository, and @gogowings who owns the ML extension.
The ML extension's source code is hosted in an external repository. I would suggest you back out the specific line change on the ML, open an issue for @gogowings to follow up.
The general suggestion is we should try to avoid merging PRs which would break CI as that would incur maintenance cost.
Of course, if @gogowings feels it easy to produce a new extension, he can submit a PR, we merge and you can rebase.

@sptramer
Copy link
Copy Markdown
Contributor Author

sptramer commented Sep 3, 2019

If I can get a URL for the ML extension, I can make a similar modification there. But I'm not sure why part of this repository's test infrastructure would be checking something from a different, disconnected repo.

@gogowings
Copy link
Copy Markdown
Contributor

Sorry for the delayed response as I am just back from vacation. As @yugangw-msft mentioned, azure-cli-ml is from an external repo and released separately. We usually release every two weeks and should have a new release this week. We can make the change for this week's CLI release so before end of the week, this will be fixed for the azure-cli-ml's project_urls, and then you can rebase.

Sounds like a plan?

@sptramer
Copy link
Copy Markdown
Contributor Author

sptramer commented Sep 3, 2019

@gogowings Sure, that's fine. Ping me in this thread again when that's fixed so I can close and reopen to force a rebuild.

@gogowings
Copy link
Copy Markdown
Contributor

The azure-cli-ml fix is in #937, waiting from approval now. FYI: @sptramer

@gogowings
Copy link
Copy Markdown
Contributor

#937 is merged.

@sptramer sptramer closed this Sep 5, 2019
@sptramer sptramer reopened this Sep 5, 2019
@sptramer
Copy link
Copy Markdown
Contributor Author

sptramer commented Sep 5, 2019

@zikalino This should now be ready to merge. Thanks!

@zikalino zikalino merged commit ab84da3 into Azure:master Sep 19, 2019
ManuInNZ pushed a commit to ManuInNZ/azure-cli-extensions that referenced this pull request Apr 11, 2020
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.

6 participants