coder/terraform-provider-coderPublic

NotificationsYou must be signed in to change notification settings
Fork24
Star52

fix: remove window option from open_in parameter#327

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

defelmnq merged 4 commits intomainfromopen_in_fix_options

Jan 20, 2025

Merged

fix: remove window option from open_in parameter#327

defelmnq merged 4 commits intomainfromopen_in_fix_options

Jan 20, 2025

Conversation

Copy link

Contributor

defelmnq commentedJan 20, 2025

The open_in parameter currently being released was first ment to containwindow as one of the options - after more review it is not relevant anymore and has to be removed.

remove window option

3463f69

defelmnq requested review fromjohnstcn andmatifali

January 20, 2025 11:08

defelmnq self-assigned this

Jan 20, 2025

defelmnq requested a review frommtojek

January 20, 2025 11:09

matifali approved these changes

Jan 20, 2025

View reviewed changes

Copy link

Member

matifali left a comment•
edited
Loading

Choose a reason for hiding this comment

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

LGTM. Thanks

@johnstcn I am a bit unsure about versioning for this change.
Technically, it is a breaking change. However, given that the added functionality in2.1.0 was never released in Codermainline orstable, it is safe to assume that no one should have been using it. And we can patch it as2.1.1.
Thoughts?

fix missing integration test

4e7dcaa

mtojek reviewed

Jan 20, 2025

View reviewed changes

integration/coder-app-open-in/main.tf OutdatedShow resolvedHide resolved

mtojek reviewed

Jan 20, 2025

View reviewed changes

integration/coder-app-open-in/main.tf

		}

		resource"coder_app""window" {
		resource"coder_app""tab" {

Copy link

Member

mtojekJan 20, 2025

Choose a reason for hiding this comment

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

The resource name istab, so we the propertycoder_app.tab.open_in is available, but you're referring tocoder_app.slim-window.open_in hence the issue?

Copy link

ContributorAuthor

defelmnqJan 20, 2025

Choose a reason for hiding this comment

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

Yes indeed , nice catch - just fixed it. 🤔

defelmnq added2 commits

January 20, 2025 12:36

fix tests

79eded5

run linter

8f259be

defelmnq requested a review frommtojek

January 20, 2025 11:45

Copy link

ContributorAuthor

defelmnq commentedJan 20, 2025

Thanks@matifali @mtojek for the review and help - this one seems ready for me -

I'll need help as@matifali said above for the versioning - not sure what's the best here.

Copy link

Member

johnstcn commentedJan 20, 2025

LGTM. Thanks
@johnstcn I am a bit unsure about versioning for this change. Technically, it is a breaking change. However, given that the added functionality in2.1.0 was never released in Codermainline orstable, it is safe to assume that no one should have been using it. And we can patch it as2.1.1. Thoughts?

Another option would be that we just makewindow default toslim-window and add a warning diagnostic. This would essentially have the same effect without being a 'breaking' change.

WDYT@defelmnq ?

mtojek approved these changes

Jan 20, 2025

View reviewed changes

Copy link

Member

mtojek left a comment

Choose a reason for hiding this comment

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

I agree it is "breaking" but as long as nobody had a chance to use, maybe we should not leave "deprecation" notice? The notice will stay with us forever...