-
Notifications
You must be signed in to change notification settings - Fork 1
Allow charon to search aliases when fetching deps #1898
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
Conversation
Check if the incoming hostname is an alias, if it's not, do the normal dep logic
| params = {} | ||
| } | ||
| var self = this | ||
| return Instance.findByIdAsync(this._id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we not add .bind(this) here and not use self?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Myztiq
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nits
lib/models/mongo/instance.js
Outdated
| throw new Instance.NotFoundError('Not masterpod and not isolated!') | ||
| } | ||
| return Instance.findOneBy(query) | ||
| .then(instance => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tap
| // Annotate dependencies with additional instance information (currently | ||
| // only adding network information for charon) | ||
| return Promise | ||
| .map(dependencies, dep => Instance.findByIdAsync(dep.instanceId)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we batch this into one fetch? This is going to be called a bunch from charon I'm guessing and speed of this is important. Unless we don't think it's a concern for the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
charon only calls it with the hostname, the UI calls for all of them, so it should only ever be 1 call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. So it's the UI call that'll be slow? Otherwise what's up with the else on line 1182, and what's the scenario it'd be ran and how that'd only be 1 array item.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was when instances had deps of themselves. This was a to remove them from showing up in the UI. I'm not sure what you mean about the 1 item array
Adding more specific errors Add unit tests
…N-5803-charon-aliases
(This is why we test)
configs/.env.development
Outdated
| REDIS_PORT=6379 | ||
| S3_CONTEXT_RESOURCE_BUCKET=runnable.context.resources.development | ||
| ALLOW_ALL_CORS=true | ||
| AWS_ALIAS_HOST=us-west-2.compute.internal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newline
lib/models/mongo/instance.js
Outdated
| method: 'InstanceSchema.methods.convertAliasesToDeps' | ||
| }) | ||
| log.info('called') | ||
| return Promise.try(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Promise.method instead of .try
Myztiq
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nits, please change those 2 things and we should be good.
Charon calls GET instance/id/dependencies/hostname
Check if the incoming hostname is an alias, if it's not, do the normal dep logic