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

[RFC] Allow use of profiles #57

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DavidGamba
Copy link
Contributor

This one is to just get your eyes on it.

The way you are doing your session creation doesn't allow for many options.
Our setup is a multi account setup where we use access keys defined in the root account and assume roles to access multiple other accounts.

In other cases we use ec2 instances with instance profiles to run our automation from.

Splitting the session creation from the resource creation allows the caller to bring custom session needs.

This will enable separation of session creation from new resource
instantiation.
It also allows callers with custom session needs to bring their own
session.
@codecov-io
Copy link

codecov-io commented Mar 13, 2020

Codecov Report

Merging #57 into master will decrease coverage by 0.71%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
- Coverage    42.2%   41.48%   -0.72%     
==========================================
  Files          59       59              
  Lines        4109     4180      +71     
==========================================
  Hits         1734     1734              
- Misses       2264     2335      +71     
  Partials      111      111
Impacted Files Coverage Δ
config/config.go 58.49% <0%> (-25.3%) ⬇️
sqs/client.go 58.53% <0%> (-3.01%) ⬇️
cloudwatch/client.go 18.75% <0%> (-5.25%) ⬇️
cloudwatch/request_type.go 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8a27a0...dbaa8f5. Read the comment docs.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 42.46% when pulling dbaa8f5 on DavidGamba:allow-use-of-profiles into 6ddcc8f on evalphobia:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 42.46% when pulling dbaa8f5 on DavidGamba:allow-use-of-profiles into 6ddcc8f on evalphobia:master.

@evalphobia evalphobia self-assigned this Mar 26, 2020
@evalphobia
Copy link
Owner

@DavidGamba Thank you for your patience and suggestion!

It's a good idea to use the session that you want.
What about Config struct directly have AWS session?
If it have the session, you don't have to change the constructor of each services. (e.g. sqs.New(conf))

/* config.go */

type Config struct {
	Session *session.Session

	AccessKey string
	...
}

 // Session creates AWS session from the Config values.
func (c Config) Session() (*session.Session, error) {
	if c.Session != nil {
		return c.Session, nil
	}
	return session.NewSession(c.AWSConfig())
}
/* sqs.go */

// New returns initialized *SQS.
func New(conf config.Config) (*SQS, error) {
	sess, err := conf.Session()
	if err != nil {
		return nil, err
	}
	...
}

@DavidGamba
Copy link
Contributor Author

Hi @evalphobia sorry for the long delay in answering.

It looks good, I need to give it a try but I wont be able to get to that in a couple of weeks.

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.

4 participants