Skip to content

fix: #32, #62#67

Merged
fox1t merged 13 commits intoducktors:mainfrom
tm1000:feature/fixes-32-62
Nov 5, 2022
Merged

fix: #32, #62#67
fox1t merged 13 commits intoducktors:mainfrom
tm1000:feature/fixes-32-62

Conversation

@tm1000
Copy link
Copy Markdown
Contributor

@tm1000 tm1000 commented Nov 2, 2022

This fixes #32 and #62

@tm1000
Copy link
Copy Markdown
Contributor Author

tm1000 commented Nov 2, 2022

The only issue I've noticed so far is that if I dont set the ENVs I added I get an error about invalid values. So if someone could direct me on how to fix that I'll do it (or I'll investigate later)

@fox1t
Copy link
Copy Markdown
Contributor

fox1t commented Nov 3, 2022

Thanks for this PR!

The only issue I've noticed so far is that if I dont set the ENVs I added I get an error about invalid values. So if someone could direct me on how to fix that I'll do it (or I'll investigate later)

I am not sure about the issue. Where do you see these errors? Can you provide the stacktrace?

@fox1t
Copy link
Copy Markdown
Contributor

fox1t commented Nov 3, 2022

Would you mind adding tests for the additions you made?

@matteovivona
Copy link
Copy Markdown
Contributor

@all-contributors add @tm1000 for code

@allcontributors
Copy link
Copy Markdown
Contributor

@tehkapa

I've put up a pull request to add @tm1000! 🎉

@tm1000
Copy link
Copy Markdown
Contributor Author

tm1000 commented Nov 3, 2022

I am not sure about the issue. Where do you see these errors? Can you provide the stacktrace?

When using docker (I rebuilt until this makes it in your release)

Error: env/STORAGE_PATH_USE_TMP_FOLDER must be boolean
    at loadAndValidateEnvironment (/home/app/node/node_modules/env-schema/index.js:93:19)
    at Object.<anonymous> (/home/app/node/src/env.ts:50:29)
    at Module._compile (node:internal/modules/cjs/loader:1126:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1180:10)
    at Module.load (node:internal/modules/cjs/loader:1004:32)
    at Function.Module._load (node:internal/modules/cjs/loader:839:12)
    at Module.require (node:internal/modules/cjs/loader:1028:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/home/app/node/src/plugins/remote-cache/storage/index.ts:4:1)
    at Module._compile (node:internal/modules/cjs/loader:1126:14) {
  errors: [
    {
      instancePath: '/STORAGE_PATH_USE_TMP_FOLDER',
      schemaPath: '#/properties/STORAGE_PATH_USE_TMP_FOLDER/type',
      keyword: 'type',
      params: { type: 'boolean' },
      message: 'must be boolean'
    }
  ]

@tm1000
Copy link
Copy Markdown
Contributor Author

tm1000 commented Nov 3, 2022

So I just rebuilt the container and I omitted BODY_LIMIT. It's marked as Optional in the env file but still throws this error unless I define it so I must be missing something

Error: env/BODY_LIMIT must be number
    at loadAndValidateEnvironment (/home/app/node/node_modules/env-schema/index.js:93:19)
    at Object.<anonymous> (/home/app/node/src/env.ts:50:29)
    at Module._compile (node:internal/modules/cjs/loader:1126:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1180:10)
    at Module.load (node:internal/modules/cjs/loader:1004:32)
    at Function.Module._load (node:internal/modules/cjs/loader:839:12)
    at Module.require (node:internal/modules/cjs/loader:1028:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/home/app/node/src/plugins/remote-cache/storage/index.ts:4:1)
    at Module._compile (node:internal/modules/cjs/loader:1126:14) {
  errors: [
    {
      instancePath: '/BODY_LIMIT',
      schemaPath: '#/properties/BODY_LIMIT/type',
      keyword: 'type',
      params: { type: 'number' },
      message: 'must be number'
    }
  ]
}

@fox1t
Copy link
Copy Markdown
Contributor

fox1t commented Nov 3, 2022

Maybe for some reason it defaults to empty string? 🤔

@tm1000
Copy link
Copy Markdown
Contributor Author

tm1000 commented Nov 3, 2022

Maybe for some reason it defaults to empty string? 🤔

That was it. Its because of the remapping happening inside of the Dockerfile. There's no need to set ENVs back to themselves as this is done automatically in docker itself when running the container so there is no point in setting empty values when building. When you end up setting booleans to undefined then it becomes a string and then it doesn't validate. I fixed this in ea614b3 and rebuilt my container and now it works without the values defined

@tm1000
Copy link
Copy Markdown
Contributor Author

tm1000 commented Nov 3, 2022

@fox1t I can write some tests if you think the code is good to go now

@fox1t fox1t requested a review from matteovivona November 4, 2022 07:30
@fox1t
Copy link
Copy Markdown
Contributor

fox1t commented Nov 4, 2022

I think that now everything seems in place! Waiting for some tests to merge this!

@fox1t fox1t merged commit ce2fd40 into ducktors:main Nov 5, 2022
@tm1000
Copy link
Copy Markdown
Contributor Author

tm1000 commented Nov 6, 2022

@fox1t I'm still working on tests if you want them! Sorry for the delay!

@matteovivona matteovivona added this to the v1.x.x milestone Feb 6, 2023
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.

Large build output cannot be uploaded

3 participants