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

Add source level schema detection as a new box #213

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PrachiChavan14
Copy link
Contributor

No description provided.

@PrachiChavan14
Copy link
Contributor Author

Any update on this ?

@toru-takahashi
Copy link
Collaborator

Sry. Let me check this.

import sys
import os
import digdag
sys.path.append('/home/td-user/.local/lib/python3.6/site-packages')
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems strange. It is for local, not server side one.

Copy link
Member

Choose a reason for hiding this comment

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

No need to have this line for the latest image digdag/digdag-python:3.7

os.system(f"{sys.executable} -m pip install --user pyyaml")
os.system(f"{sys.executable} -m pip install --user json")
os.system(f"{sys.executable} -m pip install --user requests")
os.system(f"{sys.executable} -m pip install --user digdag")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this pip command. Instead of it, please add
import digdag

Copy link
Member

Choose a reason for hiding this comment

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

With digdag/digdag-python:3.7, you don't need to use --user option anymore.

The following packages are installed:

  • pandas
  • requests
  • pytd (old version, so would be better to use pip install

also should remove the following packages:

  • json (Python standard module)
  • time (Python standard module)
  • digdag (not on PyPI)

You should write like the following:

os.system(f"{sys.executable} -m pip install -U pytd==0.8.0")
os.system(f"{sys.executable} -m pip install pyyaml")

headers = {"content-type": "application/json; charset=utf-8","authorization":"TD1 "your apikey""}
payload = json.dumps(job).encode("utf-8") if isinstance(job, dict) else job
print(payload)
with requests.post("https://api.treasuredata.com/v3/bulk_loads/guess", payload, headers=headers) as res:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Endpoint shouldn't be static. Please allow users to configure other endpoints.


def connector_guess(self, job):

headers = {"content-type": "application/json; charset=utf-8","authorization":"TD1 "your apikey""}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use apikey variable instead of writing apikey directly.

@toru-takahashi
Copy link
Collaborator

I will improve the code by myself later.

Copy link
Member

@chezou chezou left a comment

Choose a reason for hiding this comment

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

Made some feedbacks for the latest image

#Pass apikey as a secret
+secrets2:
docker:
image: "digdag/digdag-python:3.7.3-stretch"
Copy link
Member

Choose a reason for hiding this comment

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

Should use the latest image:

Suggested change
image: "digdag/digdag-python:3.7.3-stretch"
image: "digdag/digdag-python:3.7"

import sys
import os
import digdag
sys.path.append('/home/td-user/.local/lib/python3.6/site-packages')
Copy link
Member

Choose a reason for hiding this comment

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

No need to have this line for the latest image digdag/digdag-python:3.7

os.system(f"{sys.executable} -m pip install --user pyyaml")
os.system(f"{sys.executable} -m pip install --user json")
os.system(f"{sys.executable} -m pip install --user requests")
os.system(f"{sys.executable} -m pip install --user digdag")
Copy link
Member

Choose a reason for hiding this comment

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

With digdag/digdag-python:3.7, you don't need to use --user option anymore.

The following packages are installed:

  • pandas
  • requests
  • pytd (old version, so would be better to use pip install

also should remove the following packages:

  • json (Python standard module)
  • time (Python standard module)
  • digdag (not on PyPI)

You should write like the following:

os.system(f"{sys.executable} -m pip install -U pytd==0.8.0")
os.system(f"{sys.executable} -m pip install pyyaml")

out:
type: td_internal
apikey: apikey
endpoint: api.treasuredata.com
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to parameterize.

incremental: false
out:
type: td_internal
apikey: apikey
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to parameterize.

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