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

📚 Update development and testing details #41

Merged
merged 1 commit into from
Oct 18, 2022

Conversation

harshad16
Copy link
Member

@harshad16 harshad16 commented Sep 9, 2022

Update development and testing details
Signed-off-by: Harshad Reddy Nalla [email protected]

@sesheta sesheta added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 9, 2022
@VannTen
Copy link
Member

VannTen commented Sep 12, 2022

Can't we also include the smaug instance (destination) directly in the
env.template ?
Since from the documentation I get that it's intended as some sort of "default
test instance".

.env.template Outdated
Comment on lines 9 to 13

# dest ceph
THOTH_DOCUMENT_SYNC_DST="s3://thoth/data/test"
AWS_ACCESS_KEY_ID=
AWS_SECRET_ACCESS_KEY=
Copy link
Member

Choose a reason for hiding this comment

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

Assuming smaug as target, we would have :
AWS_ENDPOINT_URL=https://s3-openshift-storage.apps.smaug.na.operate-first.cloud correct ?
(This env var does not work, override the url by env var is not yet implemented in either boto3 or aws-cli, but the value are correct ?)
basing this on https://console-openshift-console.apps.smaug.na.operate-first.cloud/k8s/ns/thoth-infra-prod/configmaps/ceph

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 url is correct, i would update the documentation on where to place the endpoint as well.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what short term solution we can have regarding this. Creating a config file under $HOME (I think aws-cli / boto3 check under ~/.aws/config or something like that is not super pratictal).

Long-term I think we would want thoth-station/storages#2700

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe we could put everything in THOTH_DOCUMENT_SYNC_DST (like https://s3-openshift-storage.apps.smaug.na.operate-first.cloud/thoth/what/ever).

The lack of a standard env var in aws/boto3 is quite a pain :/

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah this is a pain, if we are using the aws cli then we can rely on --endpoint .
However as I saw with your pr, it using boto3 and python module,maybe we can use the env var for this.

for now I have include a note for current setup.

Copy link
Member

Choose a reason for hiding this comment

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

I can add an --endpoint flag with a env fallback to the command, with click it's quite trivial. (In fact I have a local commit to do just that that I used for testing).

Copy link
Member Author

Choose a reason for hiding this comment

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

that would be great, seems like a convenient option as of now.

Copy link
Member

Choose a reason for hiding this comment

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

Ok I'll add that in #37

.env.template Outdated Show resolved Hide resolved
@VannTen VannTen mentioned this pull request Sep 13, 2022
2 tasks
Comment on lines 7 to 8
THOTH_CEPH_KEY_ID=
THOTH_CEPH_SECRET_KEY=
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need credentials with this ? from python or aws-cli I get the same error -> Unable to locate credentials.

Copy link
Member Author

@harshad16 harshad16 Sep 19, 2022

Choose a reason for hiding this comment

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

yes we do, you can get the credential from the cluster.
source: https://console-openshift-console.apps.ocp4.prod.psi.redhat.com/k8s/ns/thoth-test-core/secrets/ceph
dest: https://console-openshift-console.apps.smaug.na.operate-first.cloud/k8s/ns/thoth-frontend-prod/secrets/ceph

I would like to include these detail in thoth-application docs,
so I have included the link to this in this document.

Copy link
Member

Choose a reason for hiding this comment

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

Right, this ought to be in a more general place than here 👍

.env.template Show resolved Hide resolved
.env.template Outdated Show resolved Hide resolved
Signed-off-by: Harshad Reddy Nalla <[email protected]>

Update .env.template

Co-authored-by: Max Gautier <[email protected]>
@harshad16
Copy link
Member Author

Lets get this in , wdyt @VannTen ?

@VannTen
Copy link
Member

VannTen commented Oct 18, 2022

Yeah we're good.
I think once thoth-station/thoth-application#2647 get merged we could have more precise refs (like linking directly to Credentials section) but no need to hold back this PR.
/lgtm

@sesheta sesheta added the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2022
@harshad16
Copy link
Member Author

/approve

@sesheta
Copy link
Member

sesheta commented Oct 18, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: harshad16

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sesheta sesheta added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2022
@sesheta sesheta merged commit f8052e5 into thoth-station:master Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants