Skip to content

Conversation

@funkycode
Copy link
Contributor

Here is working example.

@funkycode
Copy link
Contributor Author

As from discussion (#1) it seems like some functions were renamed. this example works perfectly for 0.5.0, probably should be adjusted according to changes in 0.5.1

Copy link
Contributor Author

@funkycode funkycode left a comment

Choose a reason for hiding this comment

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

Here is fix for 0.5.1

@olegsu
Copy link
Contributor

olegsu commented Jan 13, 2019

Thanks!
Please resolve the conflicts so we can merge

@funkycode
Copy link
Contributor Author

funkycode commented Jan 13, 2019

Btw offtopic, But as you have context and ClientOptions. I would suggest to have something like (surely I would suggest to have all values passed from context to options (below is just a sample, if you want I can create other pull request for this too, just not sure what would be your approach - maybe you would want to have New() to accept context instead)

func (c *CFContext) GetClientOptions() *ClientOptions {
	return &ClientOptions{Host: c.URL,
		Auth: AuthOptions{Token: c.Token}}
}

@funkycode
Copy link
Contributor Author

Thanks!
Please resolve the conflicts so we can merge

Done.

@olegsu
Copy link
Contributor

olegsu commented Jan 13, 2019

Have to agree with you on it #2 (comment)
I would use it right away

@olegsu olegsu merged commit 55e8665 into codefresh-io:master Jan 13, 2019
@funkycode
Copy link
Contributor Author

I'll try to push another merge request tomorrow to use context instead of options (or maybe options to have both optionally?)

@olegsu
Copy link
Contributor

olegsu commented Jan 13, 2019

In general, users probably need the ability to create a client from $HOME/.cfconfig and from options.
So I guess both will work.
Let's take it separately tomorrow?
Appreciate your contribution 👍

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.

2 participants