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

Support for specifying spec.packId on codefresh pipeline #44

Closed
wants to merge 3 commits into from

Conversation

mattmelgard
Copy link

This PR adds a resource argument to the codefresh_pipeline resource that allows a spec.packId to be provided. This is useful to users who use the built in SAAS runtime environment but who want to specify the use of a MEDIUM or LARGE resource size for that runtime environment as pictured in the UI below.

Currently this is not possible and defaults to the SMALL resource size meaning that if changes are made to a pipeline using that resource size it will be reset to the SMALL resource size upon apply.

Screen Shot 2021-04-28 at 1 48 14 PM

NOTE: I'm a new contributor to the repository and still rather new to Golang, so feel free to yell at me if I missed something important

@kpurdon
Copy link

kpurdon commented Apr 30, 2021

@alex-codefresh @vadimgusev-codefresh this is ready for review if either of you has a chance. Thanks! -- Also please let us know what the best way to request a review is in the future.

@kpurdon
Copy link

kpurdon commented Jul 22, 2021

@alex-codefresh @vadimgusev-codefresh please review or assign to whoever is responsible for that now. Thank you.

@todaywasawesome
Copy link

Looks like this PR is stuck. I'm going to huddle up with the team and figure out how we can get those moving.

@todaywasawesome
Copy link

Assigned to @ziv-codefresh

@@ -57,6 +57,7 @@ func TestAccCodefreshPipeline_Concurrency(t *testing.T) {
resource.TestCheckResourceAttr(resourceName, "spec.0.concurrency", "1"),
resource.TestCheckResourceAttr(resourceName, "spec.0.branch_concurrency", "2"),
resource.TestCheckResourceAttr(resourceName, "spec.0.trigger_concurrency", "3"),
resource.TestCheckResourceAttr(resourceName, "spec.0.pack_id", "5cd1746617313f468d669045"),

Choose a reason for hiding this comment

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

where are you getting this value from?
the tests passed for you locally?

you should create a new configuration test that will take this packId value as an argument and apply it.

Choose a reason for hiding this comment

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

@mattmelgard Wdyt?

Copy link
Author

Choose a reason for hiding this comment

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

@todaywasawesome / @ziv-codefresh I updated the tests to what I think they should look like but I'm not entirely certain on how to run the tests. Running make test seems to pass, but a good portion of them are skipped from what I can tell and I don't really see any repo-level documentation regarding how to run them either. Let me know and thanks.

@@ -71,6 +72,7 @@ func TestAccCodefreshPipeline_Concurrency(t *testing.T) {
resource.TestCheckResourceAttr(resourceName, "spec.0.concurrency", "4"),
resource.TestCheckResourceAttr(resourceName, "spec.0.branch_concurrency", "5"),
resource.TestCheckResourceAttr(resourceName, "spec.0.trigger_concurrency", "6"),
resource.TestCheckResourceAttr(resourceName, "spec.0.pack_id", "6cd1746617313f468d667048"),

Choose a reason for hiding this comment

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

same here.

@kpurdon
Copy link

kpurdon commented Dec 2, 2021

@todaywasawesome @ziv-codefresh can we please get some support on this? Thanks.

@mattmelgard
Copy link
Author

@todaywasawesome any chance we can get some updates, we'd love to get this merged in so we can continue to use the official provider. Right now this change is blocking us from doing so.

@mattmelgard
Copy link
Author

I can rebase again as needed, but I need a little guidance on some things here as well.

@mattmelgard
Copy link
Author

Looks like something might have already been merged in in the most recent release that supports specifying this now. I suppose this is probably not needed anymore?

@denis-codefresh
Copy link
Collaborator

@mattmelgard yeah, I was about to review your PR, but seems like this is already implemented in the latest commits. Anyway thanks for the effort, we will try pay more attention to this repo in the future.

@todaywasawesome
Copy link

@mattmelgard Glad we could get the feature in though wish it would have gone through the PR. I think we have some improvement to make as an open source company.

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.

5 participants