Multiple Upgrades to the Azure Ruby SDK #1064

sarangan12 · 2017-10-18T19:01:09Z

Combined all the profiles json files to one file
There were 50+ files for profile specifications, each specific to a service. This will be difficult to maintain in the future. So, With this PR, all the 50+ files have been combined to one file (and corresponding code changes to handle the merge)

Bug fix to include azure_sdk in build and release process
The build and release process did not include the azure_sdk. As a result, this gem was not included automatically in the previous release and needed manual intervention. So, With this PR, that issue has been fixed and the azure_sdk has been included in the build and release process.

Removed all the relative paths
The Metadata files and profiles files have relative paths in them which would cause issues in the future (such as moving the generator out). With this PR, all the relative paths have been removed.

Split the rollup and individual templates to get a cleaner code
The Template files for the Rollup profiles and Individual profiles were same. So, they had several if conditions all over which would definitely cause issues in the future when we want to make changes. With this PR, they are completely split and independent which would make the future enhancements easier.

Cleaned the Rakefile
The Rakefile had some unnecessary code and also complex Directory changes which will result in warnings during the regeneration. With this PR, such issues have been cleared.

None of these changes resulted in any code changes within the SDK. I have executed the bundle exec rake arm:regen && bundle exec rake arm:regen_all_profiles and this has caused 0 changes.

@veronicagg @vishrutshah @salameer @devigned Please review and approve

1. Combined all the profiles json files to one file 2. Bug fix to include azure_sdk in build and release process 3. Removed all the relative paths. 4. Split the rollup and individual templates to get a cleaner code 5. Cleaned up Rakefile

veronicagg · 2017-10-18T22:03:33Z

@sarangan12 I believe you're addressing parts of #1034. Could you split out an issue that contains only the things you planned to address with this PR? I'd like to make sure we're able to track what's pending and have issues with titles that represent what's getting addressed.

sarangan12 · 2017-10-19T21:08:14Z

Added a new commit to include code changes related to Independent Versioning of gems. Provided a short description on the changes included:

Deleted the old ARM_VERSION file.
Introduce 2 new files:
- GEM_VERSIONS
- GEMS_TO_RELEASE
What does the GEM_VERSIONS file do?
The GEM_VERSIONS file replaces the ARM_VERSION file. This file consists of all gems including their versions. This will be picked up by the regen task and the version value will be used as the value of the package-version attribute.
In addition, this file will be used by the gemspec file of the azure_sdk rollup gem.
What does the GEMS_TO_RELEASE file do?
The GEMS_TO_RELEASE file consists of the gems to be released. This will be used only by the release task.
Other Miscellaneous points:
- The regen tasks also updates the version file of the individual gems and also the version file of the azure_sdk rollup gem.
- Added require file regeneration as an enhancement to the regen task.

How will these changes impact the Jenkins Publish Job?
No significant changes. The only change will be to modify the required ARM_VERSION parameter to date to be used as the tag. I will do that change once this PR is approved.

veronicagg

Made a bunch of comments inline.
Trying to look at the pieces carefully, still have more to go for the generators, will continue and post more.

veronicagg · 2017-10-23T18:57:25Z

Rakefile


 desc 'Azure Resource Manager related tasks which often traverse each of the arm gems'
 namespace :arm do
  desc 'Delete ./pkg for each of the Azure Resource Manager projects'
  task :clean do
    each_gem do
-      execute_and_stream(OS.windows? ? 'del /S /Q pkg 2>nul' : 'rm -rf ./pkg')
+      FileUtils.rm_rf('pkg')
    end
  end



in clean_generated task below, we should enable the ability to clean by gem or based on what's in the list for generation.
Before, when generating a specific gem, we would only delete that one, now, we're deleting everything, even though the generation based on the file, may only be generating a few.

veronicagg · 2017-10-23T20:21:21Z

GEM_VERSIONS

@@ -0,0 +1,64 @@
+{


could we track all gem versions in the "config.json" file with package_version and eliminate this file?
If necessary config.json, could have a "more general" version, so we don't need to specify it for each api-version, which is actually kind of redundant (was done that way to simplify format of the file at least initially)

The missing piece if we remove this file would be azure_sdk, but I believe we could 1) have a version file in the azure_sdk folder for this "special" gem, 2) add it to config.json changing the format of the file a bit.

veronicagg · 2017-10-23T20:26:21Z

Rakefile

-version = File.read(File.expand_path('../ARM_VERSION', __FILE__)).strip
+gem_versions = JSON.parse(File.read(File.expand_path('../GEM_VERSIONS', __FILE__)).strip)
+gems_to_release = JSON.parse(File.read(File.expand_path('../GEMS_TO_RELEASE', __FILE__)).strip)
+GEMS_TO_IGNORE = ['azure_mgmt_insights']


should we just remove insights from all lists and repo? What's the intended difference between GEMS_TO_IGNORE and a combination of REGEN_EXCLUDES and GEMS_TO_RELEASE?

veronicagg · 2017-10-23T20:26:50Z

Rakefile


 desc 'Azure Resource Manager related tasks which often traverse each of the arm gems'
 namespace :arm do
  desc 'Delete ./pkg for each of the Azure Resource Manager projects'
  task :clean do
    each_gem do
-      execute_and_stream(OS.windows? ? 'del /S /Q pkg 2>nul' : 'rm -rf ./pkg')
+      FileUtils.rm_rf('pkg')


veronicagg · 2017-10-23T20:29:45Z

Rakefile

+    gems_to_release.each do |key, gems|
+      gems.each do |gem|
+        if(key == 'rollup')
+          Dir.chdir("#{__dir__}/azure_sdk")


should we be picking the name from #{gem} and not hardcode "azure_sdk"?

veronicagg · 2017-10-24T00:38:03Z

azure_sdk/azure_sdk.gemspec

@@ -5,8 +5,10 @@
 lib = File.expand_path('../lib', __FILE__)
 $LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib)



should we have this file be generated? If we add/remove gems, we would need to manually update this one. I know we missed adding some gems to the list before, so wondering.

veronicagg · 2017-10-24T00:39:07Z

azure_sdk/spec/azure_sdk_spec.rb

+      @credentials = MsRest::TokenCredentials.new(
+        MsRestAzure::ApplicationTokenProvider.new(
+          @tenant_id, @client_id, @client_secret, @active_directory_settings))
+      @options = {


per our conversation this morning with env and credentials, this may need to be updated.

veronicagg · 2017-10-24T00:47:42Z

config.json

@@ -3,87 +3,75 @@
    "azure_mgmt_analysis_services_2017_07_14": {
      "markdown": "https://raw.githubusercontent.com/Azure/azure-rest-api-specs/7aa3a5247895ba34d6cfec73e036bb66dc907d20/specification/analysisservices/resource-manager/readme.md",
      "namespace": "Azure::AnalysisServices::Mgmt::V2017_07_14",
-      "package-version": "0.15.0",


per some of my previous comments, could we have this version in this file and avoid having to add a new file for gem versions?

veronicagg · 2017-10-24T01:02:03Z

generators/profilegen/src/profile_generator.rb

@@ -69,12 +68,9 @@ def generate_modules
      @spec_includes << @module_require


could there be more than module_required?
could there be several RPs pointing to the same module_required?

veronicagg · 2017-10-24T01:04:07Z

generators/profilegen/src/profile_generator.rb

@@ -69,12 +68,9 @@ def generate_modules
      @spec_includes << @module_require
      @class_name     = get_ruby_specific_resource_type_name(resource_provider)
      @class_names   << @class_name
-      if(!@individual_gem_profile)


if this file will only be used for individual gem profile, should its name be updated to indicate this?

veronicagg

More comments inline.

veronicagg · 2017-10-30T23:47:49Z

generators/requirefilegen/src/require_file_generator_options_parser.rb

@@ -29,6 +30,10 @@ def self.parse(args)
        options.mode = mode


minor suggestion: if mode value is incorrect, we may want to raise InvalidArgument, InvalidOption seems to be applicable when the switch is undefined.

veronicagg · 2017-10-30T23:52:26Z

generators/requirefilegen/src/require_file_generator_options_parser.rb

@@ -29,6 +30,10 @@ def self.parse(args)
        options.mode = mode
      end

+      opts.on('-sSDK_PATH', '--sdk_path=SDK_PATH', 'SDK Path') do |sdk_path|


could we have a better description to what this means?, the switch name indicates it's an sdk_path, but probably not clear what the format should be (relative starting somewhere? absolute path?), it may be helpful to indicate what's being used for.

veronicagg · 2017-10-30T23:58:32Z

generators/requirefilegen/src/require_file_generator.rb

@@ -11,10 +11,10 @@ class RequireFileGenerator



can you add a description at the top of this class? The name "require file generator" doesn't seem very indicative of what this is. It contains requires, but it's really the file that gets the rest of the code loaded, is there a better name for this?
We should indicate as well, that the name of the file that gets generated is actually the gem name, and that this file is at top level in terms of the gem, and what the purpose of it is.

veronicagg · 2017-10-30T23:59:54Z

generators/requirefilegen/src/require_file_generator.rb

@@ -11,10 +11,10 @@ class RequireFileGenerator



We should not be hard-coding a gem name to exclude here, why do we need an "EXCLUDE_GEMS" in this generator? What we generate should be consistent with what gets generated by the profile generator.

Should this generator be structured to be called per gem (either rollup or individual)? So, when we want to generate new files for a gem we would call "generate profile" for a gem which would do a sub call to generate the "require file" for that specific gem, in which case, it should probably get the location where generation is producing code for that gem as input.

veronicagg · 2017-10-31T00:17:11Z

generators/requirefilegen/src/require_file_generator.rb

@@ -55,7 +55,7 @@ def initialize(azure_sdk_location, mgmt_sdks_location)
  #
  def generate_require_files_for_individual_gems
    puts 'Generating require files for individual gems'
-    Dir.chdir("#{@current_location}/#{@mgmt_sdks_location}")
+    Dir.chdir("#{@mgmt_sdks_location}")
    gems = Dir['*'].reject{|o| not File.directory?(o)}
    gems.each do |gem|
      # azure_mgmt_insights is a special case gem which we have stopped


btw, should we remove insights folder from our latest code, since we're not updating it and not shipping it?

veronicagg · 2017-10-31T01:15:26Z

generators/profilegen/src/profile_generator_options_parser.rb

@@ -13,6 +13,9 @@
 #    -h, --help : Displays help for the profile generator
 #    -d, --dir_metadata : File that contains metadata info about RPs
 #    -p, --profile : Input file contains the profile generation info about RPs, versions, etc.
+#    -m, --mode : Mode can be 'rollup'/ 'management'
+#    -k, --key : Key of the Profile


what's key of the profile?

veronicagg · 2017-10-31T01:15:45Z

generators/profilegen/src/profile_generator_options_parser.rb

@@ -13,6 +13,9 @@
 #    -h, --help : Displays help for the profile generator
 #    -d, --dir_metadata : File that contains metadata info about RPs
 #    -p, --profile : Input file contains the profile generation info about RPs, versions, etc.
+#    -m, --mode : Mode can be 'rollup'/ 'management'
+#    -k, --key : Key of the Profile
+#    -s, --sdk_path : Path to Azure Ruby SDK


can you provide more info on what the format of the path here should be?

veronicagg · 2017-10-31T01:17:06Z

generators/profilegen/src/profile_generator_options_parser.rb

@@ -13,6 +13,9 @@
 #    -h, --help : Displays help for the profile generator
 #    -d, --dir_metadata : File that contains metadata info about RPs
 #    -p, --profile : Input file contains the profile generation info about RPs, versions, etc.
+#    -m, --mode : Mode can be 'rollup'/ 'management'


would we not support "dataplane"? what would be different profile generator wise? is there a reason why the options are not rolloup & individual as in other cases?

veronicagg · 2017-10-31T01:18:51Z

generators/profilegen/src/profile_generator.rb

@@ -69,12 +68,9 @@ def generate_modules
      @spec_includes << @module_require
      @class_name     = get_ruby_specific_resource_type_name(resource_provider)
      @class_names   << @class_name
-      if(!@individual_gem_profile)
-        @default_rp_client_version = @default_versions[resource_provider]
-      end

      resource_types_obj.each do |resource_type_version, resource_types|


could you add some inline comments to help follow the logic?

veronicagg · 2017-10-31T01:20:16Z

generators/profilegen/src/profile_generator.rb

@@ -126,7 +121,7 @@ def check_available_after_index(operation_type_to_check, index_after_to_compare)
  #
  def generate_client
    file = get_client_file


similar to a comment I made in another place, should we have a method to create a file based on a name and template, seems that we do that in multiple places and not very consistent across the generator methods.

veronicagg

I left a couple of comments, please review and address as necessary before merging.
Approving conditionally on that and also on what we discussed, that you'll be creating issues for all the other comments made to address in a subsequent PR.

veronicagg · 2017-11-07T01:30:24Z

generators/profilegen/src/resources/templates/individual/client_template.template

@@ -21,15 +21,5 @@ module Azure::<%= @class_name %>::Profiles::<%= @profile_name %>::Mgmt
      super(options)
    end

-    def credentials
-      if @credentials.nil?


shouldn't this change have come as part of the merge? with merged PR https://github.com/Azure/azure-sdk-for-ruby/pull/1090/files ?
I'm curious on what else we could have missed in the merge, therefore the question.

veronicagg · 2017-11-07T01:33:40Z

Rakefile

@@ -242,7 +247,7 @@ Rake::Task['arm:regen_individual_profiles'].enhance do
  Rake::Task['arm:regen_individual_require_files'].invoke
 end

-Rake::Task['arm:regen'].enhance do
+Rake::Task['arm:regen_sdk_versions'].enhance do


change is ok. I think I made a comment on this before, on what's the benefit of enhance? could we be consistent and have the regen_sdk_versions task just call regen_individual_require_files as a pre-req?

Actually, You cannot make regen_individual_require_files a pre requisite. If you execute regen_individual_require_files before regen_sdk_versions task, the require file will be empty. Because, in order to generate the require files, the sdk versions must be generated. i.e. you need to execute regen_individual_require_files task AFTER the regen_sdk_versions task. Now, it is possible to create a third task and make both of them as pre reqs. But, according to rake specification the order is not guaranteed. So, enhancement is the only option. Also, enhancement is standard design option provided by Rake and I believe we must start using it.

sarangan12 requested review from devigned, vishrutshah, veronicagg and salameer October 18, 2017 19:01

Multiple Upgrades to the SDK:

bbc1259

1. Combined all the profiles json files to one file 2. Bug fix to include azure_sdk in build and release process 3. Removed all the relative paths. 4. Split the rollup and individual templates to get a cleaner code 5. Cleaned up Rakefile

sarangan12 force-pushed the Upgrades branch from 3278a9a to bbc1259 Compare October 18, 2017 21:26

Independent Versioning of Gems

0b67a4e

Minor Fix

f913f26

veronicagg suggested changes Oct 24, 2017

View reviewed changes

veronicagg suggested changes Oct 31, 2017

View reviewed changes

sarangan12 added 4 commits November 6, 2017 12:58

Merge branch 'master' into Upgrades

426da82

Removed ARM_VERSION file

211ff6d

Additional changes

8b777f0

Added one more comprehensive task

ce81b8a

sarangan12 mentioned this pull request Nov 6, 2017

Post Profile Release Activity #1034

Closed

veronicagg approved these changes Nov 7, 2017

View reviewed changes

sarangan12 merged commit 25433a4 into Azure:master Nov 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple Upgrades to the Azure Ruby SDK #1064

Multiple Upgrades to the Azure Ruby SDK #1064

sarangan12 commented Oct 18, 2017

veronicagg commented Oct 18, 2017

sarangan12 commented Oct 19, 2017 •

edited

Loading

veronicagg left a comment

veronicagg Oct 23, 2017

veronicagg Oct 23, 2017

veronicagg Oct 23, 2017

veronicagg Oct 23, 2017

veronicagg Oct 23, 2017

veronicagg Oct 23, 2017

veronicagg Oct 24, 2017

veronicagg Oct 24, 2017

veronicagg Oct 24, 2017

veronicagg Oct 24, 2017

veronicagg Oct 24, 2017

veronicagg left a comment

veronicagg Oct 30, 2017

veronicagg Oct 30, 2017

veronicagg Oct 30, 2017

veronicagg Oct 30, 2017

veronicagg Oct 31, 2017

veronicagg Oct 31, 2017

veronicagg Oct 31, 2017

veronicagg Oct 31, 2017

veronicagg Oct 31, 2017

veronicagg Oct 31, 2017

veronicagg Oct 31, 2017

veronicagg left a comment

veronicagg Nov 7, 2017

veronicagg Nov 7, 2017

sarangan12 Nov 7, 2017

		@@ -5,8 +5,10 @@
		lib = File.expand_path('../lib', __FILE__)
		$LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib)

		@@ -69,12 +68,9 @@ def generate_modules
		@spec_includes << @module_require

Multiple Upgrades to the Azure Ruby SDK #1064

Multiple Upgrades to the Azure Ruby SDK #1064

Conversation

sarangan12 commented Oct 18, 2017

veronicagg commented Oct 18, 2017

sarangan12 commented Oct 19, 2017 • edited Loading

veronicagg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veronicagg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veronicagg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarangan12 commented Oct 19, 2017 •

edited

Loading