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

docs: update the image in the USER_GUIDE.md #1683 #1689

Merged
merged 9 commits into from
Apr 5, 2021
Merged

docs: update the image in the USER_GUIDE.md #1683 #1689

merged 9 commits into from
Apr 5, 2021

Conversation

iamayushdas
Copy link
Contributor

Please answer these questions before submitting a pull request, or your PR will get closed.

Why submit this pull request?

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches

What changes will this PR take into?
updated the screenshots of USER_GUIDE
Please update this section with detailed description.

Related issues

fix/resolve #1683

Checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

@iamayushdas iamayushdas changed the title Docs docs: update the image in the USER_GUIDE.md #1683 Mar 30, 2021
@codecov-io
Copy link

Codecov Report

Merging #1689 (f3f595b) into master (88f3232) will increase coverage by 1.57%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1689      +/-   ##
==========================================
+ Coverage   72.41%   73.99%   +1.57%     
==========================================
  Files         133       86      -47     
  Lines        5728     2615    -3113     
  Branches      666      668       +2     
==========================================
- Hits         4148     1935    -2213     
+ Misses       1337      680     -657     
+ Partials      243        0     -243     
Flag Coverage Δ
backend-e2e-test ?
backend-e2e-test-ginkgo ?
backend-unit-test ?
frontend-e2e-test 73.99% <75.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
web/src/pages/Consumer/Create.tsx 76.08% <ø> (ø)
web/src/pages/Plugin/List.tsx 96.15% <ø> (ø)
web/src/pages/Service/Create.tsx 96.42% <ø> (+1.78%) ⬆️
web/src/pages/Upstream/Create.tsx 79.41% <50.00%> (-1.84%) ⬇️
web/src/components/Plugin/PluginDetail.tsx 65.45% <100.00%> (ø)
web/src/pages/Plugin/PluginMarket.tsx 100.00% <100.00%> (ø)
.../internal/handler/authentication/authentication.go
api/internal/filter/schema.go
api/internal/conf/conf.go
api/internal/utils/runtime/runtime.go
... and 43 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88f3232...f3f595b. Read the comment docs.

@iamayushdas
Copy link
Contributor Author

iamayushdas commented Mar 30, 2021

@nic-chen @Firstsawyou @liuxiran can you check for the failing test?

@bisakhmondal
Copy link
Member

Hii, it seems the failed test is unrelated to this PR. Can we retrigger the actions @juzhiyuan?

@iamayushdas
Copy link
Contributor Author

@iamayushdas
Copy link
Contributor Author

Hii, it seems the failed test is unrelated to this PR. Can we retrigger the actions @juzhiyuan?

Yes i think we have to retrigger

@juzhiyuan
Copy link
Member

@bisakhmondal @iamayushdas Ya, that's due to Backend, just wait for @nic-chen @starsz to fix.

@nic-chen
Copy link
Member

@iamayushdas

This is because when verifying the version number, the . In 2.4 is used as a regular expression to match all strings.

You could submit another commit to avoid this problem.

To fix this problem, we could change this line of code
https://github.com/apache/apisix-dashboard/blob/master/api/test/shell/cli_test.sh#L177
to :

if [[ `grep -c -w "${VERSION}" ${STDOUT}` -ne '1' ]]; then

I think you could submit another PR to fix it.

Thanks.

@iamayushdas
Copy link
Contributor Author

@iamayushdas

This is because when verifying the version number, the . In 2.4 is used as a regular expression to match all strings.

You could submit another commit to avoid this problem.

To fix this problem, we could change this line of code

https://github.com/apache/apisix-dashboard/blob/master/api/test/shell/cli_test.sh#L177

to :

if [[ `grep -c -w "${VERSION}" ${STDOUT}` -ne '1' ]]; then

I think you could submit another PR to fix it.

Thanks.

On it! 😊

@juzhiyuan
Copy link
Member

related PR: #1696

@iamayushdas
Copy link
Contributor Author

Could you please retrigger CI @juzhiyuan

@starsz
Copy link
Contributor

starsz commented Apr 2, 2021

Could you please retrigger CI @juzhiyuan

Since PR #1696 is merged. I think you should merge the master branch and fix the conflicts.
Then the CI can be passed.

@iamayushdas
Copy link
Contributor Author

Could you please retrigger CI @juzhiyuan

Since PR #1696 is merged. I think you should merge the master branch and fix the conflicts.

Then the CI can be passed.

Have a review to this😀

@iamayushdas
Copy link
Contributor Author

@juzhiyuan have a look to this

@@ -59,7 +59,7 @@ We can debug a route both published or offline with the online debug function, w

2. Debug a offline route

![route-debug-offline](../../assets/images/route-debug-offline.png)
![route-debug-offline](https://user-images.githubusercontent.com/40708551/112923419-e5cd7280-912b-11eb-8e7e-57c3c4fe31ef.png)
Copy link
Member

Choose a reason for hiding this comment

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

actually, it supports not only Upstream URL, but also API Gateway's Endpoint


We support the monitor page by referencing it in [iframe](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe). Before accessing [Grafana](https://grafana.com/), please Enable [`allow_embedding=true`](https://grafana.com/docs/grafana/latest/administration/configuration/#allow_embedding), which defaults to `false`. This causes the browser to fail to render Grafana pages properly due to security policies.

![metrics-en](../../assets/images/metrics-en.png)
![Dashboard-en](https://user-images.githubusercontent.com/40708551/112922395-0eed0380-912a-11eb-8c92-4c67d2bae4a8.png)
Copy link
Member

Choose a reason for hiding this comment

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

@iamayushdas thank you for this. I was wondering that where the new image file is located.

Copy link
Member

Choose a reason for hiding this comment

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

GitHub CDN?

Copy link
Member

Choose a reason for hiding this comment

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

GitHub CDN?

Yep, absolutely I think. My concern is how could us maintain these images. Previous images were maintained in this repository, while the new images seem not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iamayushdas thank you for this. I was wondering that where the new image file is located.

My pleasure sir!

Copy link
Member

Choose a reason for hiding this comment

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

🤔 that's a problem.

  • If we put them in our repo, it's easy to maintain actually, but will make the pull action slower;
  • if we put them in the CDN, we need to upload them when updating the images, but will speedup the pull action.

Both are ok to me, how about others' opinion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

☺️ #OpenSourceIsLove

Copy link
Member

Choose a reason for hiding this comment

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

Hii all, want to add something. We could also go for git lfs (faster pull). The CDN approach is okay until we need to update those static files in future releases where pinpointing the content through its CDN URL will be a huge pain in the neck. On the other hand, relative links .... (you know, just update the static file, that's it, hehe) :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but don't you think we should only go for that if we are handling large files?

Copy link
Member

Choose a reason for hiding this comment

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

Haha, there is no exact threshold to consider what is a large file, isn't it!! Just said about another available option with better docs management (IMO) :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree 😊

Copy link
Member

@imjoey imjoey left a comment

Choose a reason for hiding this comment

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

/lgtm

Thank you @iamayushdas .

@juzhiyuan juzhiyuan requested a review from LiteSun April 4, 2021 07:18
Copy link
Contributor

@liuxiran liuxiran left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot

do we need to update docs about how to upload images when someone, like me not familiar with Github CDN, wants to update the images in the user_guide? @juzhiyuan

@juzhiyuan juzhiyuan merged commit 8494e45 into apache:master Apr 5, 2021
@juzhiyuan
Copy link
Member

oad images w

ya, we could move this part to our website :)

@juzhiyuan
Copy link
Member

Anyone would like to crate an issue to track this?

@iamayushdas
Copy link
Contributor Author

Anyone would like to crate an issue to track this?

ya sure!

@iamayushdas
Copy link
Contributor Author

iamayushdas commented Apr 5, 2021

@juzhiyuan there is a problem i guess.
our website is breaking, is this because of CDN, Because it didn't happened before

See:
https://apisix.apache.org/docs/dashboard/USER_GUIDE/

here in docs its perfectly ok
https://github.com/apache/apisix-dashboard/blob/master/docs/en/latest/USER_GUIDE.md#dashboard

@juzhiyuan
Copy link
Member

alive now, that's due to we need to rebuild our website./

@1502shivam-singh
Copy link
Member

LGTM, thanks a lot

do we need to update docs about how to upload images when someone, like me not familiar with Github CDN, wants to update the images in the user_guide? @juzhiyuan

While editing the .md files on GitHub, dragging and dropping the image in should do the work :)

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.

docs: update the image in the USER_GUIDE.md document
9 participants