Skip to content

updateAzureContainernameRule : lift contianer name length restriction#140

Open
plutoshe wants to merge 3 commits into
taskgraph:masterfrom
plutoshe:updateAzureContainernameRule
Open

updateAzureContainernameRule : lift contianer name length restriction#140
plutoshe wants to merge 3 commits into
taskgraph:masterfrom
plutoshe:updateAzureContainernameRule

Conversation

@plutoshe
Copy link
Copy Markdown
Contributor

No description provided.

@xiang90
Copy link
Copy Markdown

xiang90 commented Jun 11, 2015

Can you please provide a reason for this change?

@plutoshe
Copy link
Copy Markdown
Contributor Author

It seems that azure go SDK lift the restriction of container name.
Therefore, we could name azure container more flexible

@plutoshe
Copy link
Copy Markdown
Contributor Author

The old restriction of container name is that the length must be 32.

@xiang90
Copy link
Copy Markdown

xiang90 commented Jun 11, 2015

@plutoshe Can you link the related change in the commit message or comment on github?

Comment thread filesystem/azure.go
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you can combine 57-60 to one line return err (just for your information)

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.

Yeah, it seems redundant.
Thanks for comments.

@plutoshe
Copy link
Copy Markdown
Contributor Author

I found this problem by accessing azure storage yesterday.
Maybe it's my old logic error. I wrongly think the container name length restriction.

@xiang90
Copy link
Copy Markdown

xiang90 commented Jun 11, 2015

It seems that azure go SDK lift the restriction of container name.

Your change is based on an uncertain assumption. If you can provide more information, the reviewer can have a better context of the change.

For example, you might share me which commit in azure go sdk changes the restriction (https://github.com/Azure/azure-sdk-for-go). Or how do you know the restriction changed.

@xiang90
Copy link
Copy Markdown

xiang90 commented Jun 11, 2015

Maybe it's my old logic error. I wrongly think the container name length restriction.

We want to make sure this time we can get it right. So we want to be more careful. It would be helpful if you can find any related doc in go sdk.

@plutoshe
Copy link
Copy Markdown
Contributor Author

Sure.
I don't remember why I set the this restriction before. It seems odd.
Anyway, I will find related docs.
Thanks for comments.

@xiang90
Copy link
Copy Markdown

xiang90 commented Jun 11, 2015

@plutoshe Sure. Thanks!

@plutoshe
Copy link
Copy Markdown
Contributor Author

Azure Naming Rules
When I find this docs, I find that my old code and logic is wrong here,
@xiang90 , Thanks for checking, I will check my logic and code more rigorous when I submit.
I will reimplement the name checking mechanism here.

@xiang90
Copy link
Copy Markdown

xiang90 commented Jun 11, 2015

Container Names
A container name must be a valid DNS name, conforming to the following naming rules:
Container names must start with a letter or number, and can contain only letters, numbers, and the dash (-) character.

Every dash (-) character must be immediately preceded and followed by a letter or number; consecutive dashes are not permitted in container names.

All letters in a container name must be lowercase.

Container names must be from 3 through 63 characters long.

We can start with the simplest ones: length. I am OK with adding a few TODOs.

@plutoshe
Copy link
Copy Markdown
Contributor Author

Sure, I will list some TODOs, and do the length inspection first.

@xiang90
Copy link
Copy Markdown

xiang90 commented Jun 11, 2015

@plutoshe Great. Thanks for finding the doc and putting the effort.

@plutoshe plutoshe mentioned this pull request Jun 11, 2015
4 tasks
@plutoshe plutoshe force-pushed the updateAzureContainernameRule branch from b4cd816 to 90b082e Compare June 13, 2015 09:31
@xiaoyunwu
Copy link
Copy Markdown
Contributor

why this break the test?

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.

3 participants