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

WIP: add changes to make quitstore working with eccenca DataPlatform #240

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

white-gecko
Copy link
Member

sorry, all in one ...

Copy link
Member Author

@white-gecko white-gecko left a comment

Choose a reason for hiding this comment

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

Also some PUT operation is implemented and DROP GRAPH. We will continue with the review.

quit/conf.py Outdated
@@ -110,7 +110,7 @@ def __initstoreconfig(self, namespace, upstream, targetdir, configfile):
return

def hasFeature(self, flags):
return flags == (self.features & flags)
return flags == (self.features and flags)
Copy link
Member Author

Choose a reason for hiding this comment

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

This has to be an & because the features are set as binary flags.

Choose a reason for hiding this comment

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

Yes, this is only related to changes for FEATURES

@@ -248,7 +252,7 @@ def parseArgs(args):
parser.add_argument('--flask-debug', action='store_true')
parser.add_argument('--defaultgraph-union', action='store_true')
parser.add_argument('-f', '--features', nargs='*', action=FeaturesAction,
default=Feature.Unknown,
default=feature_default,
Copy link
Member Author

Choose a reason for hiding this comment

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

The features should be set using the command line arguments instead of environment variables, if this is possible.

Choose a reason for hiding this comment

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

Is it possible to set the environment variables using Docker? I only found the git-way to set this environment variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

@seebi could you please try if you can use the --feature command line option instead of the environment variables in the orchestration (https://github.com/AKSW/QuitStore#command-line-options)?

Copy link
Member

Choose a reason for hiding this comment

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

yes, i can overwrite it in our compose file

message = queryType + ' Graph <' + graphuri + '>'
else:
message = 'New Commit from QuitStore'
oid = self.commit(graph, resultingChanges, message, parent_commit_ref,
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, 'New Commit from QuitStore' is a placeholder. It should be a message given by the client not generated based on the query, as the query is already represented in the commit message.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #237

Choose a reason for hiding this comment

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

Thanks for the tip! Yes I understand, just want to show the possibility during demonstration =)

quit/git.py Outdated
@@ -658,7 +658,8 @@ def commit(self, message, author_name, author_email, **kwargs):
commiter = pygit2.Signature(commiter_name, commiter_email)

# Sort index items
items = sorted(self.stash.items(), key=lambda x: (x[1][0], x[0]))
#items = sorted(self.stash.items(), key=lambda x: (x[1][0], x[0]))
items = list(self.stash.items())
Copy link
Member Author

Choose a reason for hiding this comment

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

@huwenxin Is it unimportant to sort the index? Could we maybe even just use self.stash.items() instead of items and use for item in items below?

Choose a reason for hiding this comment

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

During testing I found that sometimes the self.stash.items() was None so that sorted would result in error. It's also possible to use self.stash.items() instead of items below. By the way, I will list the changes/features I've made and send you later, as well as a newer version of code.

@white-gecko
Copy link
Member Author

@seebi should we merge the changes we do on master into this branch, so you have an up to date docker image?

@seebi
Copy link
Member

seebi commented Aug 21, 2019

@seebi should we merge the changes we do on master into this branch, so you have an up to date docker image?

yes, please - and thanks

@seebi
Copy link
Member

seebi commented Aug 22, 2019

for documentation, here @huwenxin file
features.pdf

@white-gecko white-gecko marked this pull request as draft January 25, 2022 21:39
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.

3 participants