-
Notifications
You must be signed in to change notification settings - Fork 22
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
base: master
Are you sure you want to change the base?
Conversation
sorry, all in one ...
There was a problem hiding this 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
quit/application.py
Outdated
@@ -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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #237
There was a problem hiding this comment.
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()) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@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 |
- Support subdirectories - fix list of stashed files
for documentation, here @huwenxin file |
sorry, all in one ...