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

TT-10659 feat: Allow disabling scratch volume #108

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jakub-bochenski
Copy link
Contributor

Description

Add a configuration option to disable mounting a volume onto tyk configuration directories.

Related Issue

Resolves #107

Motivation and Context

See #107

Test Coverage For This Change

Screenshots (if appropriate)

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)
  • Refactoring or add test (improvements in base code or adds test coverage to functionality)
  • Documentation updates or improvements.

Checklist

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side). If PRing from your fork, don't come from your master!
  • Make sure you are making a pull request against our master branch (left side). Also, it would be best if you started your change off our latest master.
  • My change requires a change to the documentation.
    • I have manually updated the README(s)/documentation accordingly.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@jakub-bochenski jakub-bochenski requested a review from a team as a code owner August 30, 2023 16:37
@jakub-bochenski jakub-bochenski requested review from komalsukhani and removed request for a team August 30, 2023 16:37
@jakub-bochenski
Copy link
Contributor Author

I've updated the PR to keep the volume only for pid file, as otherwise Tyk would complain it can't write pid file

@jakub-bochenski
Copy link
Contributor Author

would be nice to get some response here after a month :(

@buraksekili
Copy link
Member

@jakub-bochenski sorry for late response - we are at on-site event as a team. i'll raise your request internally too now.

Copy link
Member

@buraksekili buraksekili left a comment

Choose a reason for hiding this comment

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

can you please check the comments? overall, the pr looks good to me.

components/tyk-gateway/values.yaml Outdated Show resolved Hide resolved
Copy link
Member

@buraksekili buraksekili left a comment

Choose a reason for hiding this comment

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

LGTM. One small comment is that since the gateway component is updated, the umbrella charts (single-dc, mdcb-data-plane, tyk-oss) must be corrected; so that, their values.yaml file also includes the new field introduced in this PR. can you please update it? then, I'll move this ticket to QA. Thank you again for this contribution 🙏

@jakub-bochenski
Copy link
Contributor Author

Are you sure this is necessary? I think the default values from sub-charts will still be in effect.

Unless you want it there just for visibility

@buraksekili
Copy link
Member

You are right - it is not necessary and umbrella charts will be affected as you mentioned.
Just asking for visibility. If someone uses tyk-oss directly, instead of installing tyk-gateway chart directly, it'd be better to have this field also visible in umbrella charts as well.

@jakub-bochenski
Copy link
Contributor Author

I've added the values to umbrella charts

Copy link
Member

@buraksekili buraksekili left a comment

Choose a reason for hiding this comment

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

thanks a lot @jakub-bochenski!
i'll forward this ticket to QA and once the testing finished, we can go with releasing this in the upcoming releases

@jakub-bochenski
Copy link
Contributor Author

@buraksekili @komalsukhani what is the status of this PR?

@caroltyk
Copy link
Collaborator

@jakub-bochenski

We have reviewed it and will send for testing for next release. We are tight on resources right now so please expect it to be released in Q1 2024.

Thanks again for your support and contribution!

@caroltyk caroltyk changed the title feat: Allow disabling scratch volume TT-10659 feat: Allow disabling scratch volume Nov 29, 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.

Supply Tyk configuration files directly in the Docker image
4 participants