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

Removed external artifact from repository. #322

Merged
merged 5 commits into from
Oct 27, 2016

Conversation

kaizimmerm
Copy link
Contributor

First task for #197

  • Started to clean up Artifact and Software Management
  • Removed external repository

Signed-off-by: kaizimmerm kai.zimmermann@bosch-si.com

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>
Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>
Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>
Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>
@@ -122,7 +121,7 @@ public Message onAuthenticationRequest(final Message message) {
* download this artifact
*/
private void checkIfArtifactIsAssignedToTarget(final TenantSecurityToken secruityToken,
final LocalArtifact localArtifact) {
final org.eclipse.hawkbit.repository.model.Artifact localArtifact) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename the parameter localArtifact as well

@@ -146,7 +146,8 @@ private void checkByTargetId(final LocalArtifact localArtifact, final Long targe
LOG.info("download security check for target {} and artifact {} granted", targetId, localArtifact);
}

private void checkByControllerId(final LocalArtifact localArtifact, final String controllerId) {
private void checkByControllerId(final org.eclipse.hawkbit.repository.model.Artifact localArtifact,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure whether renaming LocalArtifact to Artifact is a good idea or not: As you can see in the following changes you'll get a lot of name clashes. Can't you find a better name for one of the involved classes?

Copy link
Contributor Author

@kaizimmerm kaizimmerm Oct 25, 2016

Choose a reason for hiding this comment

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

Artifact was actually always the name. LocalArtifact was a sub-interface. besides the mistake was made on DMF side. It uses repository naming schema, i.e. it should be called DmfArtifact as in the other APIs.

final MgmtArtifact artifactRest = new MgmtArtifact();
artifactRest.setType(type);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be sure: This will change the API, is it intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it was an unused part of the API. So I think its save to remove it. We supported only one type (local) anyway.

@@ -53,10 +53,19 @@
@JoinColumn(name = "software_module", nullable = false, updatable = false, foreignKey = @ForeignKey(value = ConstraintMode.CONSTRAINT, name = "fk_assigned_sm"))
private JpaSoftwareModule softwareModule;

@Column(name = "sha1_hash", length = 40, nullable = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

What about javax.validation annotations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We never do them execept if we want to do a validation that is not supported by the database

Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>
@kaizimmerm kaizimmerm merged commit b7f5bf3 into eclipse-hawkbit:master Oct 27, 2016
@kaizimmerm kaizimmerm deleted the fix_optimize_repository branch October 27, 2016 13:30
@kaizimmerm kaizimmerm modified the milestone: 0.2.0M3 Apr 3, 2017
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