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

Add ingress support #69

Merged
merged 1 commit into from
Apr 4, 2017
Merged

Add ingress support #69

merged 1 commit into from
Apr 4, 2017

Conversation

tnozicka
Copy link
Contributor

@tnozicka tnozicka commented Mar 13, 2017

  • update reference file

Closes #21

@tnozicka
Copy link
Contributor Author

@kadel please review this when you get a chance. The WIP is just because it's missing an update to reference doc. I'll fix it once we agree that the implementation is ok.


// TODO: Add Uri unmarshalling to validate it

type UriPath string
Copy link
Contributor

Choose a reason for hiding this comment

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

What is difference between Uri and UriPath ?

Copy link
Member

Choose a reason for hiding this comment

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

Same question here.
Also calling it uri might be little bit misleading :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also calling it uri might be little bit misleading :(

@kadel could you elaborate on this?

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 guess it should be named URN if that's what you meant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@surajssd @kadel you are right, calling it even URN is to broad. Host should be Fqdn (as defined by RFC 3986) and path can also be a regexp so it should be PathRegex (as defined by IEEE Std 1003.1)

}

for _, tt := range tests {
t.Run("", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please name the tests?

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 don't want to. Naming just for the sake of naming is wrong. Naming is hard and I would rather have unnamed test than those with misleading descriptions. Especially with the feature that if you specify empty string there and it fails, it will tell you the exact test index. good enough for me.

This feature is for generic naming when you can actually substitute the test case into some formatted string like fmt.Sprintf("%s in %s", tc.gmt, tc.loc) - not for structures taking 20 lines to initialize.

Copy link
Contributor

Choose a reason for hiding this comment

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

how can a description that the test author write be misleading? since the test author has highest knowledge of what is happening.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because naming is hard ;) (especially when it's suppose to make sense to others)

result := []runtime.Object{}

i := &ext_v1beta1.Ingress{
ObjectMeta: api_v1.ObjectMeta{
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we abstract this ObjectMeta ? Now it's used in 4 places already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really sure what you would like to get abstracted here. The object data are always different.

Copy link
Contributor

Choose a reason for hiding this comment

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

places like these: 1, 2, 3

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 don't think there is anything to abstract there, it's just object initialization

@tnozicka tnozicka changed the title WIP - Add ingress support Add ingress support Mar 22, 2017
@tnozicka
Copy link
Contributor Author

tnozicka commented Mar 22, 2017

Updated the confusing Uri and added ingress support to reference-file

@kadel PTAL


Specifying host will make your application accessible outside of the cluster on domain specified as a value of `host`. (Note: this requires you to manually setup DNS records to point to your cluster's Ingress router. For development you can use services like http://nip.io/ or [http://xip.io/].)

#### path
Copy link
Member

@kadel kadel Mar 23, 2017

Choose a reason for hiding this comment

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

I think we need a little bit more explanation what path does. Ideally with some examples.

@tnozicka
Copy link
Contributor Author

tnozicka commented Apr 4, 2017

@kadel I've added a description on path based routing to address #69 (comment)

@kadel kadel merged commit 2f09159 into redhat-developer:master Apr 4, 2017
@tnozicka tnozicka deleted the add-ingress-support branch April 5, 2017 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants