Azure-Samples / modern-data-warehouse-dataops

DataOps for Microsoft Data Platform technologies. https://aka.ms/dataops-repo
MIT License
592 stars 463 forks source link

Feat/single tech/feature engineering on fabric br/setup env notebook #654

Closed siliang-j-1225 closed 1 year ago

siliang-j-1225 commented 1 year ago

Add optional setup environment notebook for simplified Azure resource setup.

Type of PR

Purpose

Add a deploy.ipynb notebook to assist setting up environment in feature engineering pipeline of Fabric.

Does this introduce a breaking change? If yes, details on what can break

No.

Author pre-publish checklist

Validation steps

Follow instructions in the notebook to create environment.

Issues Closed or Referenced

No.

Additional Information

Immigrated from this PR https://github.com/Azure-Samples/modern-data-warehouse-dataops/pull/653

promisinganuj commented 1 year ago

@thurstonchen , @siliang-j-1225 , this is something new to me and I like it a lot. My concern is that it would add a bit of confusion to the person reading through the instructions.

I would propose to just add a one-line footnote at the end of the "Required resources" section and put a link to the notebook there.

Few other comments:

siliang-j-1225 commented 1 year ago

@thurstonchen , @siliang-j-1225 , this is something new to me and I like it a lot. My concern is that it would add a bit of confusion to the person reading through the instructions.

I would propose to just add a one-line footnote at the end of the "Required resources" section and put a link to the notebook there. --> Removed the previous paragraph and added a link at the end of the "Required resources" section

Few other comments:

  • Move the "deploy.ipynb" to "src/infra/deploy-azure-resources.ipynb". -->done
  • Remove Step 2 and Step 3 completely from the notebook, these instructions are already present in the main README.md and we want people to use this notebook as a standalone artifact. -->done
  • For the following comments in the notebook:
run jupyter notebooks
authenticate with credential of the target tenant

We would either need to provide some more guidance about it or remove it altogether. -->decided to remove it altogether. For example, the reader would be wondering about which Azure service to use. -->added new contents in the beginning of the notebook shows which services will be created and what roles will be assigned

  • In the notebook, please add all the details about what it does, how it does in the markdown so that it's self-contained. I would expect people to also use it as a reference point for such a unique deployment method. -->I added some more details to markdown for each cell
  • Also remove "Step 1. Create Azure Resources" header as it won't be required with the new change. -->done

Hey @promisinganuj, thanks for the informative comment! I have made some updates of the notebook and readme based on your comment. Please have a look later.

promisinganuj commented 1 year ago

Hi @siliang-j-1225 , thanks for the changes. I pushed few minor modifications and I am happy for this PR to be merged now. Well done!