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

Copy over annotations to ingress #7

Merged
merged 2 commits into from
Sep 8, 2019
Merged

Copy over annotations to ingress #7

merged 2 commits into from
Sep 8, 2019

Conversation

MarcusNoble
Copy link
Contributor

@MarcusNoble MarcusNoble commented Jul 8, 2019

Signed-off-by: Marcus Noble m.noble@elsevier.com

Description

Copy the annotations from FunctionIngress to the Ingress object that gets created so that ingress-specific annotations can be used (for example marking the ingress as being internal)

Motivation and Context

I am currently looking to expose a function within out work network, using an internal ALB but an annotation on the ingress resource is needed for this.

How Has This Been Tested?

Test yaml:

apiVersion: openfaas.com/v1alpha2
kind: FunctionIngress
metadata:
  name: example
  namespace: openfaas
  annotations:
    zalando.org/aws-load-balancer-scheme: internal
spec:
  domain: example.test.com
  function: example
  ingressType: skipper

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Impact to existing users

There shouldn't be any impact.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@derek derek bot added the new-contributor label Jul 8, 2019
@derek derek bot added the no-dco label Jul 9, 2019
@derek
Copy link

derek bot commented Jul 9, 2019

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

@alexellis
Copy link
Member

Please could you reset/squash the commits down to remove the merge commit?

That function should be easy to test. I wrote some tests but they don't appear to be in the repo.

Try this: https://blog.alexellis.io/golang-writing-unit-tests/

@derek derek bot removed the no-dco label Jul 10, 2019
Signed-off-by: Marcus Noble <m.noble@elsevier.com>
Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

Looks in great shape, just a couple of nits for maintenance now, a separate commit will be fine for these changes.

Thanks for adding the unit tests, you didn't resolve, or reply to the comment. That's something you can do to help me.

@derek derek bot added the no-dco label Aug 8, 2019
@derek
Copy link

derek bot commented Aug 8, 2019

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

Signed-off-by: Marcus Noble <m.noble@elsevier.com>
@derek derek bot removed the no-dco label Aug 8, 2019
@MarcusNoble
Copy link
Contributor Author

@alexellis Are there any more changes requested for this PR?

}
annotations["kubernetes.io/ingress.class"] = class
annotations["com.openfaas.spec"] = string(specJSON)
Copy link
Member

Choose a reason for hiding this comment

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

@MarcusNoble should you take specJSON after setting the ingress.class?

@alexellis
Copy link
Member

Thank you for picking this up again :)

I have left a comment

@alexellis alexellis merged commit 0c6a3fd into openfaas:master Sep 8, 2019
@alexellis
Copy link
Member

Thank you @MarcusNoble 💪 0.4.0 is now out with the changes.

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.

2 participants