-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
🎉 New Source: SAP Fieldglass [low-code cdk] #18656
Conversation
Tested the connector using sandbox url and works. Do I need to shift to production url ? Production needs login details. |
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.
Hello @Sid-Lais, Marcos from Airbyte here 👋 . We received more than 25 new contributions along the weekend. One is yours 🎉 thank so much for! Our team is limited and maybe the review process can take longer than expected. As described in the Airbyte's Hacktoberfest your contribution was submitted before November 2nd and it is eligible to win the prize. The review process will validate other requirements. I ask to you patience until someone from the team review it.
Because I reviewed some contributions for Hacktoberfest so far I saw some common patterns you can check in advance:
- Make sure you have added connector documentation to
/docs/integrations/
- Remove the file
catalog
from/integration_tests
- Edit the
sample_config.json
inside/integration_tests
- For the
configured_catalog
you can use onlyjson_schema: {}
- Add title to all properties in the
spec.yaml
- Make sure the
documentationUrl
in thespec.yaml
redirect to Airbyte's future connector page, eg: connector Airtable thedocumentationUrl: https://docs.airbyte.com/integrations/sources/airtable
- Review now new line at EOF (end-of-file) for all files.
If possible send to me a DM in Slack with the tests credentials, this process will make easier to us run integration tests and publish your connector. If you only has production keys, make sure to create a bootstrap.md explaining how to get the keys.
It isn't necessary to use productions keys. |
So, I've tested using sandbox url and it works. Testing gives 2 errors for now. Can't test production as it needs login details I guess. Gives connection error.
Second error says Primary key is null. I've used the right primary key given in documentation. The sandbox url does give some null primary keys. |
@girarda it is allow to use nullable primary keys? |
@Sid-Lais could you share the test account credentials with me? |
Shared the credentials on slack |
@Sid-Lais I'm having issue running |
Build completed with 3 failures. It says old python used even though I have python3.10
|
@marcosmarxm @Sid-Lais - it's not possible to have a nullable primary key, but it's possible to not set a primary key |
It doesn't let me not set a primary key
|
Hello! I'm going to be out of the office this Friday and won't be able to review your contribution again today, I return next Monday. So far, most contributions look solid and are almost done to be approved. As said in Chris' comment all contributions made before 2-November are eligible to receive the prize and have 2 weeks to merge the contributions. But I ensure next week we're going to have your contribution merged. If you have questions about the implementation you can send them in Sorry the inconvenience and see you again next week, thank you so much for your contribution! |
No worries. Enjoy your weekend. (◍•ᴗ•◍) |
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.
@Sid-Lais I think this source must change the name to SAP Fieldgrass to make clear is only one specific module of SAP.
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.
Some comments, the error you pointed is related about your schema.
airbyte-integrations/connectors/source-sap/source_sap/schemas/TODO.md
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-sap/source_sap/schemas/data.json
Outdated
Show resolved
Hide resolved
So all the files also has to be changed to sap_fieldglass from sap? |
Yes. |
@marcosmarxm Rebuilt the whole connector from ground and turns out I had somewhat used old generator. Everything works now and all test passed. Test result added to the PR in acceptance section. |
Done |
/test connector=connectors/source-sap-fieldglass
Build PassedTest summary info:
|
This reverts commit 413dedff6d8eced2c6c07bf3aa849eb6dce5b30a.
Fixed formating
/publish connector=connectors/source-sap-fieldglass
if you have connectors that successfully published but failed definition generation, follow step 4 here |
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 @Sid-Lais
* Generated boilerplate * Connector woring in sandbox * Connector working with 1 record from stream * Sample state * Added docs * Revert "Added docs" This reverts commit 413dedff6d8eced2c6c07bf3aa849eb6dce5b30a. * Added docs finally * Updated config * Fixed data.json and cleaned some files * Changed name from sap to sap-fieldglass and fixed test errors * Re-added Primary key for future use * Removed primary key as test failed * Update settings.json Fixed formating * add sap fieldglass to source def * auto-bump connector version * remove oss_catalog.json * revert change to linting file * run format Co-authored-by: marcosmarxm <marcosmarxm@gmail.com> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com> Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com>
What
Added source connector for SAP. Issue
How
Using
low code cdk
Recommended reading order
docs/integrations/sources/sap-fieldglass.md
spec.yaml
sap.yaml
🚨 User Impact 🚨
No change to base code
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereTests
Acceptance