Skip to content

Improvement suggestions #395

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.

Sign up for GitHub

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

Merged
beckermarc merged 9 commits into from
Nov 25, 2024
Merged

Improvement suggestions #395

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
22f1e79
Add an example route mapping to README.md
maginseb Nov 22, 2024
81b1b4e
Fix error when creating a transaction in bookshop-srv (MT)
maginseb Nov 22, 2024
c435b17
Add missing dev-dependency in db package.json
maginseb Nov 22, 2024
3fff7bd
Replace spaces with tabs.
maginseb Nov 22, 2024
7b3ce88
Merge branch 'main' into suggestion/d060326
mofterdinger Nov 22, 2024
6336110
Revert "Replace spaces with tabs."
maginseb Nov 22, 2024
9511d42
Revert "Fix error when creating a transaction in bookshop-srv (MT)"
maginseb Nov 22, 2024
c9e7392
Merge branch 'SAP-samples:main' into suggestion/d060326
maginseb Nov 25, 2024
6636f83
Remove example and replace it with parameter descriptions
maginseb Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ Deploy as Multitenant Application:
- Run `cf deploy mta_archives/bookshop-mt_1.0.0.mtar`
- Go to another subaccount in your global account, under subscriptions and subscribe to the application you deployed.
- Run `cf map-route bookshop-mt-app <YOUR DOMAIN> --hostname <SUBSCRIBER TENANT>-<ORG>-<SPACE>-bookshop-mt-app` or create and bind the route manually.
- Example: `cf map-route bookshop cfapps.us10.hana.ondemand.com --hostname subscriber1-myOrg-mySpace-bookshop-mt-app`
Copy link
Contributor

@beckermarc beckermarc Nov 25, 2024

Choose a reason for hiding this comment

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

Does this example really help? What is the additional value/information you want to convey with that?

I am always a little hesitant to have such CLI snippets here, as they are usually copied without much thinking and then people are confused, if they don't work.

Copy link
Contributor Author

@maginseb maginseb Nov 25, 2024

Choose a reason for hiding this comment

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

You are right that examples bear the risk to be mindlessly copied. However, for someone like me who wasn't entirely sure what I was looking for, seeing a real example was incredibly helpful.

In this instance, it helped me to see the expected format of <YOUR DOMAIN>. I found this information here.

I can try to include some more information about the assumptions for the example or where to find the respective information. If you think it's not valuable I can also remove it from the PR.

Copy link
Contributor

@beckermarc beckermarc Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

okay, maybe it would be better in that case to provide a description where to obtain the value for <YOUR DOMAIN>, e.g.:

Suggested change
- Example: `cf map-route bookshop cfapps.us10.hana.ondemand.com --hostname subscriber1-myOrg-mySpace-bookshop-mt-app`
- `<YOUR DOMAIN>`: Find the app domain for your landscape by executing `cf domains`. It commonly starts with `cfapps.`.

Copy link
Contributor Author

@maginseb maginseb Nov 25, 2024

Choose a reason for hiding this comment

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

Please have another look on the change. I have followed your suggestion and replaced the example with a description for the parameters that I struggled with during my setup.

Copy link
Contributor

@beckermarc beckermarc Nov 25, 2024

Choose a reason for hiding this comment

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

thanks 👍


> [!NOTE]
> Please note that the route length is limited to 63 characters and can easily be exceeded. So keeping the app name and sub-account subdomain as short as possible will help you stay within length.
Expand Down
Loading