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

Feature/aem remote assets #1707

Conversation

HitmanInWis
Copy link
Contributor

This is reopen of #1294 - the PR was previously approved, but was lacking junit tests and thus went stale and never got merged.

As a reminder, this is a PR for the "AEM Remote Assets" feature presented at 2018 Adobe Summit AEM Rockstar.

PR now has strong junit code coverage, and has been tested to work on AEM 6.3 and 6.4.

There are multiple use cases for this feature.

  • Keeping Dev/QA/Stage servers up to date with Production assets
  • Local servers used for development or other business functions
  • Assets stored in a non-Adobe DAM or even a PIM (NOTE: this would require further development to implement "connectors" to those different systems)
  • Standalone AEM Assets instance with separate AEM Sites instance (until Adobe releases a solution for this)

We currently have a client requesting this to be installed on their DEV servers to keep assets in sync w/o pulling the entire DAM, so we're looking forward to getting this officially merged and potentially enhanced further. I've also had other companies reach out to me directly regarding this feature.

One change I am considering is renaming this feature to be more accurate to what it is. Though the Summit Rock Star feature was termed "Remote Assets", perhaps this would better be termed "Asset Auto-Sync" or something along those lines. That way if AEM were to ever support assets from a truly remote DAM to be used directly it would avoid confusion.

HitmanInWis and others added 30 commits March 1, 2018 13:45
…complete but proves out the pattern and includes the main pieces necessary for the feature
…rs (such as for DAM Update Assets) can choose to ignore events from, so that we can work w/assets without triggering unnecessary workflows.
… the admin user runs subsystems like resource property observation which can then trigger unexpected binary syncs
…LUTIONS/acs-aem-commons into feature/aem-remote-assets

# Conflicts:
#	bundle/src/main/java/com/adobe/acs/commons/remoteassets/impl/RemoteAssetsConfigImpl.java
#	bundle/src/main/java/com/adobe/acs/commons/remoteassets/impl/RemoteAssetsNodeSyncImpl.java
…LUTIONS/acs-aem-commons into feature/aem-remote-assets

# Conflicts:
#	bundle/src/main/java/com/adobe/acs/commons/remoteassets/impl/RemoteAssetsConfigImpl.java
#	bundle/src/main/java/com/adobe/acs/commons/remoteassets/impl/RemoteAssetsNodeSyncImpl.java
…sting binaries and different temporary assets.
…orrectly convert Double props that come over in JSON as strings.
Updated remote asset sync of date properties to maintain timezone
…cleaner logging, handle jpg/jpeg, closed streams.
…, only create binaries if the lastModified has changed.
@HitmanInWis
Copy link
Contributor Author

@davidjgonzalez why is this being closed versus left open with the "Do NOT merge until 4.0.0 release" label like the other currently active PRs?

@HitmanInWis
Copy link
Contributor Author

@davidjgonzalez Now that 4.0.0 is released, can this be reopened?

@HitmanInWis
Copy link
Contributor Author

@badvision @davidjgonzalez FWIW I was trying to get this completed in time for Summit this year, as I know people will ask me about it. One very large company even reached out to me with their team of engineers looking for this feature. Even if 4.1.0 wont be officially released by Summit, it would be great if I could point people to ACS Commons and indicate it will be in the next release.

@badvision badvision reopened this Mar 11, 2019
@badvision
Copy link
Contributor

I think this was closed only because we had a lot of small fires to put out. Do we have a pr in the docs project to explain this one? I think the dev/QA use cases were rather compelling!

@HitmanInWis
Copy link
Contributor Author

Sounds good @badvision, thanks. I'm going to get onto the documentation task next - finally had time to sort out a couple final issues on this PR. Will link to it when ready, hopefully in the next day or two.

@HitmanInWis
Copy link
Contributor Author

The single remaining codeclimate issue I dont know a good way to resolve without making the code more obscure - to me its most clear the way it is, but happy to consider refactoring.

I plan to have the documentation PR up this afternoon or sometime tomorrow. Just waiting on creative team to create a thumbnail for the feature :)

@HitmanInWis
Copy link
Contributor Author

And yay...if this gets merged I get to lay claim to bringing ACS Commons code coverage over 50% 😛

@HitmanInWis
Copy link
Contributor Author

Documentation PR complete!

@badvision
Copy link
Contributor

@HitmanInWis -- any chance you can check into that switch statement complaint from Code Climate?

@HitmanInWis
Copy link
Contributor Author

@badvision The single remaining codeclimate issue I dont know a good way to resolve without making the code more obscure - to me its most clear the way it is, but if you insist it must be addressed I can take another look.

@HitmanInWis
Copy link
Contributor Author

For example, I could put a bunch of callback functions (or objects with a function) into a map, fetch them from the map by key, and then call the function, but I'm not sure how much more "clear" that makes the code. In some ways it could be considered more obscure that way.

@badvision
Copy link
Contributor

I'm okay with adding a comment to disable that check for that one method. ;)

…older(), as refactoring will likely not make the code any more straightforward.
@badvision
Copy link
Contributor

Alright. Here we go! 😄

@badvision badvision merged commit 3bc7de6 into Adobe-Consulting-Services:master Mar 22, 2019
@HitmanInWis
Copy link
Contributor Author

weeeee, thanks @badvision ! It's been a long time coming, just in time for Summit :)

@HitmanInWis HitmanInWis deleted the feature/aem-remote-assets branch May 2, 2019 22:06
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.

6 participants