duckdb / duckdb_azure

Azure extension for DuckDB
MIT License
51 stars 17 forks source link

Add proxy configuration support #31

Closed quentingodeau closed 10 months ago

quentingodeau commented 10 months ago

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

quentingodeau commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@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

quentingodeau commented 10 months ago

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

samansmink commented 10 months ago

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

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