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

UI: Fix IPv6 task links #5992

Closed
wants to merge 13 commits into from
Closed

UI: Fix IPv6 task links #5992

wants to merge 13 commits into from

Conversation

backspace
Copy link
Contributor

@backspace backspace commented Jul 22, 2019

This updates the href of task row links to handle IPv6 addresses properly; see the hover address in the corner:

image

@@ -76,37 +76,58 @@ module('Acceptance | allocation detail', function(hooks) {
});

test('each task row should list high-level information for the task', async function(assert) {
const task = server.db.taskStates.where({ allocationId: allocation.id }).sortBy('name')[0];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was only checking the first task, but since there can only be one IP per task, I need at least two tasks to check both V4 and V6 IPs, so this now loops over the tasks.

@backspace backspace marked this pull request as ready for review July 23, 2019 16:33
@backspace
Copy link
Contributor Author

The Mirage scenario override is so you can visit a demonstration here, I’ll remove before merging.

Copy link
Contributor

@DingoEatingFuzz DingoEatingFuzz left a comment

Choose a reason for hiding this comment

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

I think the display of the address should also include the brackets.

I did some perusing of the spec which states that all ipv6 urls should include brackets, port or otherwise.

To use a literal IPv6 address in a URL, the literal address should be
enclosed in "[" and "]" characters. For example the following
literal IPv6 addresses:

  FEDC:BA98:7654:3210:FEDC:BA98:7654:3210
  1080:0:0:0:8:800:200C:4171
  3ffe:2a00:100:7031::1
  1080::8:800:200C:417A
  ::192.9.5.5
  ::FFFF:129.144.52.38
  2010:836B:4179::836B:4179

would be represented as in the following example URLs:

  http://[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]:80/index.html
  http://[1080:0:0:0:8:800:200C:417A]/index.html
  http://[3ffe:2a00:100:7031::1]
  http://[1080::8:800:200C:417A]/foo
  http://[::192.9.5.5]/ipng
  http://[::FFFF:129.144.52.38]:80/index.html
  http://[2010:836B:4179::836B:4179]

Given this, I'm wondering if there's an opportunity to push the bracket logic up into the model or serializer?

ui/app/components/task-row-port.js Show resolved Hide resolved
ui/tests/acceptance/allocation-detail-test.js Outdated Show resolved Hide resolved
@backspace
Copy link
Contributor Author

I wasn’t sure about what it should look like in the link text, as I don’t know that I’ve come across such displays without the http://.

I can move it into the serialiser. I searched and only found one other use of .ip, this template, which actually needs the same fix as this!

@backspace backspace changed the title Fix IPv6 task links UI: Fix IPv6 task links Jul 23, 2019
@backspace
Copy link
Contributor Author

I’m closing this in favour of #6007, which addresses the problem at the serialiser level.

@backspace backspace closed this Jul 24, 2019
@backspace backspace deleted the b-ui/ipv6-task-addresses branch July 24, 2019 15:21
@github-actions
Copy link

github-actions bot commented Feb 6, 2023

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants