You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/08/31 00:26:40 UTC
[GitHub] [airflow-client-go] houqp opened a new pull request #5: add test & doc for authentication
houqp opened a new pull request #5:
URL: https://github.com/apache/airflow-client-go/pull/5
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow-client-go] feluelle commented on a change in pull request #5: add test & doc for authentication
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #5:
URL: https://github.com/apache/airflow-client-go/pull/5#discussion_r480987786
##########
File path: client_test.go
##########
@@ -0,0 +1,70 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package client_test
+
+import (
+ "context"
+ "encoding/base64"
+ "fmt"
+ "net/http"
+ "net/http/httptest"
+ "net/url"
+ "testing"
+
+ "github.com/apache/airflow-client-go/airflow"
+ "github.com/stretchr/testify/assert"
+)
+
+func TestBasicAuth(t *testing.T) {
+ // Start a local HTTP server
+ server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+ assert.Equal(
+ t,
+ fmt.Sprintf(
+ "Basic %s",
+ base64.StdEncoding.EncodeToString([]byte("username"+":"+"password")),
Review comment:
```suggestion
base64.StdEncoding.EncodeToString([]byte("username:password")),
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow-client-go] houqp edited a comment on pull request #5: add test & doc for authentication
Posted by GitBox <gi...@apache.org>.
houqp edited a comment on pull request #5:
URL: https://github.com/apache/airflow-client-go/pull/5#issuecomment-687704588
@mik-laj I don't have GCP to test google openid auth, but one should be able to set the token using https://github.com/apache/airflow-client-go/blob/master/airflow/configuration.go#L54. As for Kerberos, openapi-generator doesn't support it out of the box.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow-client-go] houqp commented on a change in pull request #5: add test & doc for authentication
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #5:
URL: https://github.com/apache/airflow-client-go/pull/5#discussion_r481308612
##########
File path: .github/workflows/ci.yml
##########
@@ -46,8 +46,10 @@ jobs:
- name: Set up Go
uses: actions/setup-go@v1
with:
- go-version: 1.14
+ go-version: 1.15
id: go
- name: build & test
- run: cd airflow && go test
+ run: |
+ go test
+ cd airflow && go test
Review comment:
the first command runs the manually written e2e tests, the second command runs the generated tests from openapi-generator if there is any.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow-client-go] mik-laj commented on pull request #5: add test & doc for authentication
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #5:
URL: https://github.com/apache/airflow-client-go/pull/5#issuecomment-687442219
Have you tested how the client is generated for Google OpenID or Kerberos?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow-client-go] houqp commented on a change in pull request #5: add test & doc for authentication
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #5:
URL: https://github.com/apache/airflow-client-go/pull/5#discussion_r483190395
##########
File path: .github/workflows/ci.yml
##########
@@ -46,8 +46,10 @@ jobs:
- name: Set up Go
uses: actions/setup-go@v1
with:
- go-version: 1.14
+ go-version: 1.15
id: go
- name: build & test
- run: cd airflow && go test
+ run: |
+ go test
+ cd airflow && go test
Review comment:
good call, will make that change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow-client-go] turbaszek commented on a change in pull request #5: add test & doc for authentication
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #5:
URL: https://github.com/apache/airflow-client-go/pull/5#discussion_r480994097
##########
File path: .github/workflows/ci.yml
##########
@@ -46,8 +46,10 @@ jobs:
- name: Set up Go
uses: actions/setup-go@v1
with:
- go-version: 1.14
+ go-version: 1.15
id: go
- name: build & test
- run: cd airflow && go test
+ run: |
+ go test
+ cd airflow && go test
Review comment:
Why we run tests two times?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow-client-go] turbaszek commented on a change in pull request #5: add test & doc for authentication
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #5:
URL: https://github.com/apache/airflow-client-go/pull/5#discussion_r481382539
##########
File path: .github/workflows/ci.yml
##########
@@ -46,8 +46,10 @@ jobs:
- name: Set up Go
uses: actions/setup-go@v1
with:
- go-version: 1.14
+ go-version: 1.15
id: go
- name: build & test
- run: cd airflow && go test
+ run: |
+ go test
+ cd airflow && go test
Review comment:
What would you say to make a Makefile to run tests? It seems a common way, but no strong opinion
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow-client-go] houqp commented on pull request #5: add test & doc for authentication
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #5:
URL: https://github.com/apache/airflow-client-go/pull/5#issuecomment-684057510
@feluelle it would be 1.13, that's when go has the module feature fully backed into the official tooling.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow-client-go] turbaszek commented on a change in pull request #5: add test & doc for authentication
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #5:
URL: https://github.com/apache/airflow-client-go/pull/5#discussion_r480994510
##########
File path: go.mod
##########
@@ -0,0 +1,8 @@
+module github.com/apache/airflow-client-go
+
+go 1.13
Review comment:
On CI we use `1.15` should we upgrade also here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow-client-go] turbaszek commented on a change in pull request #5: add test & doc for authentication
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #5:
URL: https://github.com/apache/airflow-client-go/pull/5#discussion_r481810597
##########
File path: .github/workflows/ci.yml
##########
@@ -46,8 +46,10 @@ jobs:
- name: Set up Go
uses: actions/setup-go@v1
with:
- go-version: 1.14
+ go-version: 1.15
id: go
- name: build & test
- run: cd airflow && go test
+ run: |
+ go test
+ cd airflow && go test
Review comment:
> the first command runs the manually written e2e tests, the second command runs the generated tests from openapi-generator if there is any.
Eventually, we may consider splitting those test into two steps to get better feedback, but that's a visual improvement
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow-client-go] houqp commented on pull request #5: add test & doc for authentication
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #5:
URL: https://github.com/apache/airflow-client-go/pull/5#issuecomment-687704588
@mik-laj I don't have GCP to test openid auth, but one should be able to set the token using https://github.com/apache/airflow-client-go/blob/master/airflow/configuration.go#L54. As for Kerberos, openapi-generator doesn't support it out of the box.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow-client-go] houqp merged pull request #5: add test & doc for authentication
Posted by GitBox <gi...@apache.org>.
houqp merged pull request #5:
URL: https://github.com/apache/airflow-client-go/pull/5
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow-client-go] houqp commented on a change in pull request #5: add test & doc for authentication
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #5:
URL: https://github.com/apache/airflow-client-go/pull/5#discussion_r481719209
##########
File path: .github/workflows/ci.yml
##########
@@ -46,8 +46,10 @@ jobs:
- name: Set up Go
uses: actions/setup-go@v1
with:
- go-version: 1.14
+ go-version: 1.15
id: go
- name: build & test
- run: cd airflow && go test
+ run: |
+ go test
+ cd airflow && go test
Review comment:
i also don't have a strong opinion here. i usually use makefile for my own projects, but since Jarek has a strong opinion on this let's just not use it for now. I am also very hesitate to use heavy tools like bazel for simple tasks like this. i am going to leave it as is for now since it's just invoking simple `go test` command. But if any one of you have ideas on how we should further automate this, i am more than happy to update my PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow-client-go] potiuk commented on a change in pull request #5: add test & doc for authentication
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #5:
URL: https://github.com/apache/airflow-client-go/pull/5#discussion_r481464286
##########
File path: .github/workflows/ci.yml
##########
@@ -46,8 +46,10 @@ jobs:
- name: Set up Go
uses: actions/setup-go@v1
with:
- go-version: 1.14
+ go-version: 1.15
id: go
- name: build & test
- run: cd airflow && go test
+ run: |
+ go test
+ cd airflow && go test
Review comment:
Just a comment here : even being an old-timer and Bash-afficionado, I'd strongly discourage Makefiles to any new projects in favour of more modern tools.
Especially if you do not plan to use Makefile's build dependency management (which is the powerful part). In most part simple bash script is enough for simple cases, and in more complex ones https://bazel.build/ seems to be so much better choice and already pretty standard.
Also, the approach where you have separate commands/scripts in Ci depending on each other is good enough in most cases IMHO.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow-client-go] houqp commented on a change in pull request #5: add test & doc for authentication
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #5:
URL: https://github.com/apache/airflow-client-go/pull/5#discussion_r481310058
##########
File path: go.mod
##########
@@ -0,0 +1,8 @@
+module github.com/apache/airflow-client-go
+
+go 1.13
Review comment:
we would want to keep this version as low as possible for ease of adoption. I will add a new job in CI to test run 1.13 as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org