-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
toml page - replace li by table #1995
toml page - replace li by table #1995
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jmaitrehenry,
thank you for this contribution, I like the table based style much more :)
I added some comments about the parts you added outside of the table transformation. Can you explain those?
docs/toml.md
Outdated
# | ||
Cluster = "default" | ||
Clusters = ["default"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused by this change. I can't see any notion of Clusters
in the code. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, the current branch seems to be out-of-date with the master.
Only he file docs/toml.md
is up-to-date that's why these differences appear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, then I will wait for the rebase and have a look again.
docs/toml.md
Outdated
# Optional | ||
# Default: false | ||
# | ||
AutoDiscoverClusters = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same as in my previous comment here.
docs/toml.md
Outdated
@@ -1780,6 +1797,8 @@ Træfik needs the following policy to read ECS information: | |||
"Sid": "Traefik ECS read access", | |||
"Effect": "Allow", | |||
"Action": [ | |||
"ecs:ListClusters", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any calls to ListClusters
or DescribeClusters
in the ecs provider code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmaitrehenry Many thanks for your contribution.
Can you rebase please?
47fc794
to
43b60af
Compare
@nmengin @marco-jantke I just push the rebase In an another PR I will move more li to table if you want. |
@jmaitrehenry Could you use a table formatting tool (like tablesgenerator)? |
@ldez done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Related to #1993