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

[MARLIN-615] Add ability to specify sync from date in synchronization job #187

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

Conversation

iseessel
Copy link

@iseessel iseessel commented Jan 9, 2019

No description provided.

@iseessel
Copy link
Author

iseessel commented Jan 9, 2019

Majority of changes are just changes in variable names. Only real code change is here: https://github.com/maestrano/maestrano-connector-rails/pull/187/files#diff-f0257db6f246dbf7a243a8b6fe2bdef7R68 plus the specs for this behavior change.


it 'passes the correct sync_from dates' do
organization.synchronized_entities.each do |entity, _|
expect_any_instance_of(Maestrano::Connector::Rails::SynchronizationJob).to receive(:sync_entity)
Copy link
Author

@iseessel iseessel Jan 9, 2019

Choose a reason for hiding this comment

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

Really just need to test the sync_from. Tested entity.to_s and organization because their readily available.

The other arguments don't relate to this option and would be a pain to test. Don't think its necessary.

Copy link

@alexmaestrano alexmaestrano left a comment

Choose a reason for hiding this comment

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

LGTM, nice surgical changes

@alexmaestrano
Copy link

Just the Travis CI failing tests

@iseessel iseessel force-pushed the improvement/marlin-615-add-ability-to-sync-delta branch 2 times, most recently from 3ec9ed8 to f6ed35c Compare January 9, 2019 20:51
Copy link
Contributor

@ouranos ouranos left a comment

Choose a reason for hiding this comment

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

LGTM

@alexmaestrano
Copy link

This will not make it into tonight's release, since we're choosing to move forward with https://github.com/maestrano/marlin-banking-adapter/pull/49 for now

@iseessel
Copy link
Author

@ouranos Can we push to have this tested and deployed soon? It would require reverting: https://github.com/maestrano/marlin-banking-adapter/pull/49 and changing the parameters on the hourly sync.

@iseessel iseessel force-pushed the improvement/marlin-615-add-ability-to-sync-delta branch from f6ed35c to ec42d06 Compare March 12, 2019 15:05
@iseessel
Copy link
Author

iseessel commented Mar 12, 2019

@ouranos I rebased and removed my travis commit like you said, but now my travis build is failing.

@iseessel
Copy link
Author

@ouranos @alexmaestrano Can you review latest changes.

@iseessel iseessel force-pushed the improvement/marlin-615-add-ability-to-sync-delta branch from 38c8035 to 4045c30 Compare March 12, 2019 16:29
Copy link
Contributor

@ouranos ouranos left a comment

Choose a reason for hiding this comment

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

LGTM

For the Travis build is seems to be similar to the problem we have on mnoe, I'll have a look

@ouranos ouranos requested a review from hhemanth March 13, 2019 00:47
@ouranos
Copy link
Contributor

ouranos commented Mar 13, 2019

@iseessel see #189

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.

None yet

4 participants