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.

Sign up for GitHub

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

Jump to bottom

feat: namespace globally configurable #1352

Merged
merged 6 commits into from
Oct 31, 2022
Merged

feat: namespace globally configurable #1352

merged 6 commits into from
Oct 31, 2022

Conversation

lkingland
Copy link
Member

@lkingland lkingland commented Oct 18, 2022 •
edited by lance
Loading

🎁 default namespace globally configurable
🎁 namespace flag help more contetually relavant
🧹 removes namespace from commands where it is not applicable

Moves namespace derivation logic to global config and prepares commands for integration by defining namespace directly on each.

Related: #901

/kind enhancement

* default namespace is now globally configurable
* namespace flag help is more contextually relevant
* removes the namespace flag from commands where it is not applicable

@knative-prow knative-prow bot added kind/techdebt do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 18, 2022
@codecov
Copy link

codecov bot commented Oct 18, 2022 •
edited
Loading

Codecov Report

Base: 53.49% // Head: 53.49% // No change to project coverage 👍

Coverage data is based on head (b187308) compared to base (b187308).
Patch has no changes to coverable lines.

❗ Current head b187308 differs from pull request most recent head 1ba5310. Consider uploading reports for the commit 1ba5310 to get more accurate results

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1352   +/-   ##
=======================================
  Coverage   53.49%   53.49%           
=======================================
  Files          72       72           
  Lines       10474    10474           
=======================================
  Hits         5603     5603           
  Misses       4444     4444           
  Partials      427      427

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

lance
lance reviewed Oct 19, 2022
View reviewed changes
Copy link
Member

@lance lance left a comment

Choose a reason for hiding this comment

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

Looks good @lkingland! Thank you. I only have a couple of very small nits.

config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/config_test.go Outdated Show resolved Hide resolved
@lance
Copy link
Member

lance commented Oct 19, 2022

@lkingland is this meant to be a draft PR?

@lkingland
Copy link
Member Author

@lkingland is this meant to be a draft PR?

Yes, it is dependent on 1344, and will need to be rebased when that is merged

@lkingland lkingland changed the title src: expanded config namespace test [WIP] src: expanded config namespace test Oct 21, 2022
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2022
@lkingland lkingland force-pushed the lkingland/config-namespace-tests branch from 768ac56 to 42f9503 Compare October 28, 2022 14:43
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2022
@knative-prow knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 28, 2022
@lkingland lkingland force-pushed the lkingland/config-namespace-tests branch 2 times, most recently from 83c22ec to d6b325d Compare October 28, 2022 15:55
@lkingland lkingland changed the title [WIP] src: expanded config namespace test [WIP] feat: namespace globally configurable Oct 28, 2022
@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 28, 2022
@lkingland lkingland marked this pull request as ready for review October 29, 2022 02:21
@lkingland lkingland changed the title [WIP] feat: namespace globally configurable feat: namespace globally configurable Oct 29, 2022
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 29, 2022
@lkingland lkingland force-pushed the lkingland/config-namespace-tests branch from d1c5746 to d8d10c2 Compare October 31, 2022 11:25
lance
lance reviewed Oct 31, 2022
View reviewed changes
Copy link
Member

@lance lance left a comment

Choose a reason for hiding this comment

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

Overall looks good. I have a small nit and a few questions.

cmd/deploy.go Outdated Show resolved Hide resolved
cmd/info.go Outdated Show resolved Hide resolved
cmd/root_test.go Show resolved Hide resolved
@lance
Copy link
Member

lance commented Oct 31, 2022

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 31, 2022
@lkingland lkingland force-pushed the lkingland/config-namespace-tests branch from ae6d2a5 to 1ba5310 Compare October 31, 2022 17:21
@knative-prow knative-prow bot removed lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 31, 2022
@lkingland
Copy link
Member Author

Rebased to resolve merge conflict

@lance
Copy link
Member

lance commented Oct 31, 2022

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 31, 2022
@lkingland lkingland added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 31, 2022
@knative-prow
Copy link

knative-prow bot commented Oct 31, 2022

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: lkingland

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot merged commit 0fa9359 into knative:main Oct 31, 2022
@lkingland lkingland added this to the 1.8.0 Release milestone Nov 22, 2022
lance pushed a commit to lance/func that referenced this pull request Feb 15, 2023
@lkingland @lance
feat: namespace globally configurable (knative#1352)
8bf8174 
* namespace global config

* integrate namespace config into commands

* comment updates

* combine config write tests

* updates per code review

* regen docs
@lkingland lkingland deleted the lkingland/config-namespace-tests branch March 6, 2023 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lance lance lance left review comments

Assignees

@lance lance

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/enhancement lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Functions WG Roadmap
Archived in project
Milestone
1.8.0 Release
Development

Successfully merging this pull request may close these issues.
3 participants
@lkingland @lance @knative-prow-robot