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

Related to Issue 1005 in Strongbox: Addition of checkSumsMap property to liquibase #8

Closed
wants to merge 1 commit into from

Conversation

ogryniuk
Copy link
Contributor

Added an xml file with the details of a new property

@carlspring
Copy link
Member

Is there any actual code in this pull request? The file seems to be empty.

Also, checksum is one word, so could you please make sure the property is named correctly (checksumsMap)? Thanks!


<changeSet id="v1.0.0.19" author="[email protected]">

<o:createProperty name="checkSums" type="linkmap" className="ArtifactEntry"/>
Copy link
Member

Choose a reason for hiding this comment

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

the type should be embeddedmap as follows:

type="embeddedmap" linkedType="String"

@sbespalov
Copy link
Member

sbespalov commented Mar 15, 2019

@ogryniuk can you please also provide a screenshot from OrientDB Studio with ArtifactEntry schema?
This needed to be sure that changeset correctly applied and mapped into OreintDB native schema.
By default you can access OrientDB Studio with http://localhost:2480/studio/index.html.

@ogryniuk
Copy link
Contributor Author

@ogryniuk can you please also provide a screenshot ...?

@sbespalov I understand why we must have a screenshot but would need an advice.

If I am right, OrientDb starts from a strongbox snapshot (at least, it is the way I started it recently).

But snapshot is a tested working version of the project published on GitHub and my changes to the liquibase would evidently be absent from it. They would only be on my machine and nowhere else.

So, how may I proceed in this situation?

@sbespalov
Copy link
Member

sbespalov commented Mar 15, 2019

But snapshot is a tested working version of the project published on GitHub and my changes to the liquibase would evidently be absent from it.

this is not required, you only need to have your strongbox-db snapshot version installed into your local .m2 repository. So just strongbox-db$ mvn clean install should be enough.

@ogryniuk
Copy link
Contributor Author

just strongbox-db$ mvn clean install should be enough.

@sbespalov I tried it but the browser did not load anything. Any ideas about what to do? I already looked into Firewall and Proxy settings, saw nothing unusual. Also had set to true an environment variable STRONGBOX_ORIENTDB_STUDIO_ENABLED, again with no result.
Opera Snapshot_2019-03-15_123125_localhost

@sbespalov
Copy link
Member

sbespalov commented Mar 15, 2019 via email

@ogryniuk
Copy link
Contributor Author

Can you join our rocket chat?

@sbespalov Sure

@ogryniuk
Copy link
Contributor Author

ogryniuk commented Mar 16, 2019

@sbespalov

Here is the snapshot of ArtifactEntry schema
Screenshot from 2019-03-19 12-18-52


<changeSet id="v1.0.0.19" author="[email protected]">

<o:createProperty name="checkSums" type="embeddedmap" linkedType="String" className="ArtifactEntry"/>
Copy link
Member

@carlspring carlspring Mar 16, 2019

Choose a reason for hiding this comment

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

As discussed previously, this should be checksums, not checkSums. (Could you also please change this in the ArtifactEntry class?).

Also, as author, could you please use the system id that we're using -- [email protected]? Thanks! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good evening @carlspring ,
First I changed the variable name the way you asked but then looked at OrientDB schema for this entity (ArtifactEntry) and saw that all the properties in fact follow the naming convention checkSums (not checksums). See the snapshot attached above.

Thus, do not you think it is better to rename it back to checkSums?

Copy link
Member

Choose a reason for hiding this comment

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

If this is the case, then you've spotted something that needs to be corrected. Could you please changes those to checksums as well? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

@ogryniuk I believe you see the database state from the moment when you initially used checkSums word instead of checksums. If you drop db directory from the strongbox-web-core, install strongbox-db with checksums, and then restart the strongbox app I believe the database will be setup using proper field naming.

Copy link
Contributor Author

@ogryniuk ogryniuk Mar 18, 2019

Choose a reason for hiding this comment

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

If this is the case, then you've spotted something that needs to be corrected. Could you please changes those to checksums as well?

@carlspring I would need a clarification to be sure I will do what you wish. So, you would like that I remove capitalisation from all the properties in ArtifactEntry xml?

For each of the following properties: storageId, repositoryId, artifactCoordinates, tagSet. artifactPath, sizeInBytes, downloadCount, lastUpdatedlastUsed?

Is it so? Please confirm

Copy link
Member

Choose a reason for hiding this comment

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

@ogryniuk ,

We are not talking about the decapitalization of everything. Checksum is one word, therefore it should not be checkSums. Please, follow @fuss86 advice! Thanks! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@carlspring
In such case, know that it is already renamed checksums, as you can see it in the updated snapshot

Copy link
Member

Choose a reason for hiding this comment

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

Okay, thanks for changing it! :)

@carlspring
Copy link
Member

@ogryniuk ,

What's the status of this task?

@ogryniuk
Copy link
Contributor Author

@carlspring
The status is that it is done (since 9 days).
However, as it is up to you to accept it or not: As you know, this pull request is dependent on other changes which are still pending - there are still tests to be done for the addition of a checksum and I keep looking for a way to do them.

@carlspring
Copy link
Member

@ogryniuk ,

Thanks for the update! We require tests when code is changed, or added.

@sbespalov , @fuss86 : Could you please give @ogryniuk some hints / links on how to implement some tests for this? Thanks!

@ogryniuk
Copy link
Contributor Author

ogryniuk commented Apr 5, 2019

Closed in favour of #10

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