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 proxy configuration support #31

Merged
merged 11 commits into from
Jan 29, 2024
Merged

Conversation

quentingodeau
Copy link
Contributor

No description provided.

@quentingodeau
Copy link
Contributor Author

Hello team!
First PR to this repository, do not hesitate if something is no clear or is incorrect. I try to follow the way configuration elements where done and I hope that I didn't miss something.
Regards,
Quentin

Copy link
Collaborator

@samansmink samansmink left a comment

Choose a reason for hiding this comment

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

Hi @quentingodeau! thanks for the PR! I think your code looks good!

I have one general comment though, I would like to see some test cases here. Could you maybe add a simple http proxy to CI and write some sql tests?

@quentingodeau
Copy link
Contributor Author

Yes of course will try to do that :)
I just have to see how to run that, it might take sometime before i update this as I'm doing this on my free time

@quentingodeau quentingodeau marked this pull request as draft January 23, 2024 23:28
@quentingodeau
Copy link
Contributor Author

I adding the tests when I'm done I will change back the PR from draft to ready to review.

Just one question how can I define an env variable only for one test and run it ?
Do i create a separated step where I set the env and call for example something:

    - name: Test extension
      env:
        HTTP_PROXY: http://localhost:3128
#      if: ${{ matrix.arch == 'linux_amd64_gcc4' || matrix.arch == 'linux_amd64'}}
      if: ${{ matrix.arch == 'linux_amd64_gcc4' }}
      run: |
        ./build/release/test/unittest /mnt/d/gitws/duckdb_azure/test/sql/azure_proxy_local_secrets.test 

or do you have some recommendation (I'm not yet familiar with the action part) ?

thx in advance.
Quentin

@quentingodeau
Copy link
Contributor Author

Greetings!

I have updated the tests, but I will need help for the Windows part if you have some time to tell/help me set up them.
For the MacOS part it's only guesses from what I have read on internet, but I do not have a Mac at hand to run the tests...
I'm still unsure about how I can handle the case of the tests with the env variable (HTTP_PROXY)

Once everything is ok, please before merging let me squash a bit my commits.
Regards,
Quentin

@samansmink
Copy link
Collaborator

Hi @quentingodeau thanks a lot! This looks great.

lets skip testing on windows i don't think its worth it to set that up. Also Regarding the HTTP_PROXY env var: I'm fine with leaving the tests as is, I think this is sufficient for now.

If CI is all green, feel free to squash, then I think this is good to go

@quentingodeau quentingodeau marked this pull request as ready for review January 25, 2024 20:42
@quentingodeau
Copy link
Contributor Author

Hello @samansmink !

I hope that the new commit will fix the squid incompatibility that I saw in the workflow logs.

There is something that I found a bit strange when you have run the previous workflow (linux failed with squid incompatibility, for MacOS I mess up the indentation) but windows has worked... I was expected to easier need to install squid or do as it has been done in some others test add an environment variable as a guard to avoid to run the tests.

Regards,
Quentin

@quentingodeau
Copy link
Contributor Author

Hi @samansmink,

So far I was able to see what I have done wrong with the CI but for this one I have no clue :(

Regards,
Quentin

@samansmink
Copy link
Collaborator

@quentingodeau no worries, thats an upstream issue we need to fix. Its caused by the ccache action bumping the node version to one that is incompatible with the manylinux2014 images glibc. I'll fix that later

Copy link
Collaborator

@samansmink samansmink left a comment

Choose a reason for hiding this comment

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

LGTM! thanks a lot for the work here

@samansmink samansmink merged commit fb2799b into duckdb:main Jan 29, 2024
9 of 12 checks passed
@quentingodeau
Copy link
Contributor Author

Thx a lot (too bad I didn't rebade to have a clean history) I will do better next time!

@samansmink
Copy link
Collaborator

Once everything is ok, please before merging let me squash a bit my commits.

Ah mb sorry I forgot, it's all good though!

@quentingodeau quentingodeau deleted the feature/proxy branch February 3, 2024 01:06
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.

2 participants